Re: [PATCH v3 02/10] xen/version: Introduce non-truncating deterministically-signed XENVER_* subops

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 16 Aug 2023, Andrew Cooper wrote:
> On 16/08/2023 1:19 am, Stefano Stabellini wrote:
> > On Tue, 15 Aug 2023, Andrew Cooper wrote:
> >> @@ -498,6 +499,59 @@ static int __init cf_check param_init(void)
> >>
> >> +    sz = strlen(str);
> >> +
> >> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running 
> >> operations. */
> >> +        return -E2BIG;
> > Realistically do we want this buffer to cross page boundaries?
> 
> A 1-byte answer can cross a page boundary, even if the hypercall
> argument is correctly aligned (and even that is unspecified in the Xen ABI).
> 
> But importantly, this series is also prep work to fixing the cmdline
> limit.  1k is already causing problems, and 64k is a whole lot less bad
> than 4k when we enter such corner cases.

OK. It is just that 64K seems *a lot* in this context. But if you have
reasons to believe that 64K is a good number for this check then OK.


> >> + */
> >>  #define XENVER_commandline 9
> >>  typedef char xen_commandline_t[1024];
> >>  
> >> @@ -110,6 +133,42 @@ struct xen_build_id {
> >>  };
> >>  typedef struct xen_build_id xen_build_id_t;
> >>  
> >> +/*
> >> + * Container for an arbitrary variable length buffer.
> >> + */
> >> +struct xen_varbuf {
> >> +    uint32_t len;                          /* IN:  size of buf[] in 
> >> bytes. */
> >> +    unsigned char buf[XEN_FLEX_ARRAY_DIM]; /* OUT: requested data.        
> >>  */
> > I realize that you just copied struct xen_build_id
> 
> No - it was originally an ambiguously-signed char in v1.  It became
> unsigned through review.
> 
> But being unsigned char is relevant to the non-ABI-changingness of later
> patches in the series.
> 
> > but I recall from
> > MISRA C training that we should use plain "char" for strings for good
> > reasons, not "unsigned char"?
> 
> I don't recall anything to that effect, nor can I see anything obvious
> when scanning through the standard.
> 
> MISRA can't plausibly prohibit the use of char for arbitrary data.  It's
> the one and only thing the C spec states is safe for the task.

After this morning's call with Roberto, I take this comment back.
unsigned char is OK from MISRA POV.