| Message ID | 20201105221905.1350-4-dbuono@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Control-Flow Integrity | expand |
On 11/5/20 11:18 PM, Daniele Buono wrote: > The UASStatus data structure has a variable sized field inside of type uas_iu, > that however is not placed at the end of the data structure. > > This placement triggers a warning with clang 11, and while not a bug right now, > (the status is never a uas_iu_command, which is the variable-sized case), > it could become one in the future. The problem is uas_iu_command::add_cdb, indeed. > > ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] If possible remove the "../qemu-base/" as it does not provide any useful information. > uas_iu status; > ^ > 1 error generated. > > Fix this by moving uas_iu at the end of the struct Your patch silents the warning, but the problem is the same. It would be safer/cleaner to make 'status' a pointer on the heap IMO. > > Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> > --- > hw/usb/dev-uas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c > index cec071d96c..5ef3f4fec9 100644 > --- a/hw/usb/dev-uas.c > +++ b/hw/usb/dev-uas.c > @@ -154,9 +154,9 @@ struct UASRequest { > > struct UASStatus { > uint32_t stream; > - uas_iu status; > uint32_t length; > QTAILQ_ENTRY(UASStatus) next; > + uas_iu status; > }; > > /* --------------------------------------------------------------------- */ >
Hi Philippe, On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote: > On 11/5/20 11:18 PM, Daniele Buono wrote: >> The UASStatus data structure has a variable sized field inside of type uas_iu, >> that however is not placed at the end of the data structure. >> >> This placement triggers a warning with clang 11, and while not a bug right now, >> (the status is never a uas_iu_command, which is the variable-sized case), >> it could become one in the future. > > The problem is uas_iu_command::add_cdb, indeed. > >> >> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] > > If possible remove the "../qemu-base/" as it does not provide > any useful information. > Sure, will do at the next cycle >> uas_iu status; >> ^ >> 1 error generated. >> >> Fix this by moving uas_iu at the end of the struct > > Your patch silents the warning, but the problem is the same. > It would be safer/cleaner to make 'status' a pointer on the heap IMO. I'm thinking of moving 'status' in a pointer with the following code changes: UASStatus is allocated in `usb_uas_alloc_status`, which currently does not take a type or size for the union field. I'm thinking of adding requested size for the status, like this: static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id, uint16_t tag, size_t size); and the common call would be usb_uas_alloc_status([...],sizeof(uas_iu)); Also we'd need a double free when the object is freed. Right now it's handled in the code when the object is not used anymore with a `g_free(st);`. I'd have to replace it with `g_free(st->status); g_free(st);`. Would you suggest doing it place or by adding a usb_uas_dealloc_status() function? --- However, I am confused by the use of that variable-lenght field. I'm looking at what seems the only place where a command is parsed, in `usb_uas_handle_data`. uas_iu iu; [...] switch (p->ep->nr) { case UAS_PIPE_ID_COMMAND: length = MIN(sizeof(iu), p->iov.size); usb_packet_copy(p, &iu, length); [...] break; [...] It would seem that the copy is limited to at most sizeof(uas_iu), so even if we had anything in add_cdb[], that wouldn't be copied here? Is this intended? > >> >> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> >> --- >> hw/usb/dev-uas.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c >> index cec071d96c..5ef3f4fec9 100644 >> --- a/hw/usb/dev-uas.c >> +++ b/hw/usb/dev-uas.c >> @@ -154,9 +154,9 @@ struct UASRequest { >> >> struct UASStatus { >> uint32_t stream; >> - uas_iu status; >> uint32_t length; >> QTAILQ_ENTRY(UASStatus) next; >> + uas_iu status; >> }; >> >> /* --------------------------------------------------------------------- */ >> >
Hi On Thu, Nov 19, 2020 at 8:19 PM Daniele Buono <dbuono@linux.vnet.ibm.com> wrote: > Hi Philippe, > > On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote: > > On 11/5/20 11:18 PM, Daniele Buono wrote: > >> The UASStatus data structure has a variable sized field inside of type > uas_iu, > >> that however is not placed at the end of the data structure. > >> > >> This placement triggers a warning with clang 11, and while not a bug > right now, > >> (the status is never a uas_iu_command, which is the variable-sized > case), > >> it could become one in the future. > > > > The problem is uas_iu_command::add_cdb, indeed. > > > >> > >> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with > variable sized type 'uas_iu' not at the end of a struct or class is a GNU > extension [-Werror,-Wgnu-variable-sized-type-not-at-end] > > > > If possible remove the "../qemu-base/" as it does not provide > > any useful information. > > > Sure, will do at the next cycle > >> uas_iu status; > >> ^ > >> 1 error generated. > >> > >> Fix this by moving uas_iu at the end of the struct > > > > Your patch silents the warning, but the problem is the same. > > It would be safer/cleaner to make 'status' a pointer on the heap IMO. > > I'm thinking of moving 'status' in a pointer with the following code > changes: > > UASStatus is allocated in `usb_uas_alloc_status`, which currently does > not take a type or size for the union field. I'm thinking of adding > requested size for the status, like this: > > static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id, > uint16_t tag, size_t size); > > and the common call would be > usb_uas_alloc_status([...],sizeof(uas_iu)); > > Also we'd need a double free when the object is freed. Right now > it's handled in the code when the object is not used anymore with a > `g_free(st);`. > I'd have to replace it with > `g_free(st->status); g_free(st);`. Would you suggest doing it place > or by adding a usb_uas_dealloc_status() function? > > --- > > However, I am confused by the use of that variable-lenght field. > I'm looking at what seems the only place where a command is > parsed, in `usb_uas_handle_data`. > > uas_iu iu; > [...] > switch (p->ep->nr) { > case UAS_PIPE_ID_COMMAND: > length = MIN(sizeof(iu), p->iov.size); > usb_packet_copy(p, &iu, length); > [...] > break; > [...] > > It would seem that the copy is limited to at most sizeof(uas_iu), > so even if we had anything in add_cdb[], that wouldn't be copied > here? > > Is this intended? > > Any update on this patch? thanks -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 19, 2020 at 8:19 PM Daniele Buono <<a href="mailto:dbuono@linux.vnet.ibm.com">dbuono@linux.vnet.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Philippe,<br> <br> On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote:<br> > On 11/5/20 11:18 PM, Daniele Buono wrote:<br> >> The UASStatus data structure has a variable sized field inside of type uas_iu,<br> >> that however is not placed at the end of the data structure.<br> >><br> >> This placement triggers a warning with clang 11, and while not a bug right now,<br> >> (the status is never a uas_iu_command, which is the variable-sized case),<br> >> it could become one in the future.<br> > <br> > The problem is uas_iu_command::add_cdb, indeed.<br> > <br> >><br> >> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]<br> > <br> > If possible remove the "../qemu-base/" as it does not provide<br> > any useful information.<br> > <br> Sure, will do at the next cycle<br> >> uas_iu status;<br> >> ^<br> >> 1 error generated.<br> >><br> >> Fix this by moving uas_iu at the end of the struct<br> > <br> > Your patch silents the warning, but the problem is the same.<br> > It would be safer/cleaner to make 'status' a pointer on the heap IMO.<br> <br> I'm thinking of moving 'status' in a pointer with the following code<br> changes:<br> <br> UASStatus is allocated in `usb_uas_alloc_status`, which currently does<br> not take a type or size for the union field. I'm thinking of adding<br> requested size for the status, like this:<br> <br> static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id,<br> uint16_t tag, size_t size);<br> <br> and the common call would be<br> usb_uas_alloc_status([...],sizeof(uas_iu));<br> <br> Also we'd need a double free when the object is freed. Right now<br> it's handled in the code when the object is not used anymore with a<br> `g_free(st);`.<br> I'd have to replace it with<br> `g_free(st->status); g_free(st);`. Would you suggest doing it place<br> or by adding a usb_uas_dealloc_status() function?<br> <br> ---<br> <br> However, I am confused by the use of that variable-lenght field.<br> I'm looking at what seems the only place where a command is<br> parsed, in `usb_uas_handle_data`.<br> <br> uas_iu iu;<br> [...]<br> switch (p->ep->nr) {<br> case UAS_PIPE_ID_COMMAND:<br> length = MIN(sizeof(iu), p->iov.size);<br> usb_packet_copy(p, &iu, length);<br> [...]<br> break;<br> [...]<br> <br> It would seem that the copy is limited to at most sizeof(uas_iu),<br> so even if we had anything in add_cdb[], that wouldn't be copied<br> here?<br> <br> Is this intended?<br> <br></blockquote><div><br></div><div>Any update on this patch?</div><div>thanks</div><div></div></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 1/14/2021 3:17 AM, Marc-André Lureau wrote: > > Any update on this patch? > thanks Hi, I had been waiting for a feedback on my previous message. I don't know the UAS subsystem that well, but can't find where the variable-sized field that is causing the issue is actually used. If it helps, I can send an RFC for converting struct UASStatus { uint32_t stream; uas_iu status; uint32_t length; QTAILQ_ENTRY(UASStatus) next; }; to struct UASStatus { uint32_t stream; uas_iu *status; uint32_t length; QTAILQ_ENTRY(UASStatus) next; }; And discuss it at that point. Thanks, Daniele
On 11/19/20 5:16 PM, Daniele Buono wrote: > Hi Philippe, > > On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote: >> On 11/5/20 11:18 PM, Daniele Buono wrote: >>> The UASStatus data structure has a variable sized field inside of >>> type uas_iu, >>> that however is not placed at the end of the data structure. >>> >>> This placement triggers a warning with clang 11, and while not a bug >>> right now, >>> (the status is never a uas_iu_command, which is the variable-sized >>> case), >>> it could become one in the future. >> >> The problem is uas_iu_command::add_cdb, indeed. >> >>> >>> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with >>> variable sized type 'uas_iu' not at the end of a struct or class is a >>> GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] >> >> If possible remove the "../qemu-base/" as it does not provide >> any useful information. >> > Sure, will do at the next cycle >>> uas_iu status; >>> ^ >>> 1 error generated. >>> >>> Fix this by moving uas_iu at the end of the struct >> >> Your patch silents the warning, but the problem is the same. >> It would be safer/cleaner to make 'status' a pointer on the heap IMO. > > I'm thinking of moving 'status' in a pointer with the following code > changes: > > UASStatus is allocated in `usb_uas_alloc_status`, which currently does > not take a type or size for the union field. I'm thinking of adding > requested size for the status, like this: > > static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id, > uint16_t tag, size_t size); > > and the common call would be > usb_uas_alloc_status([...],sizeof(uas_iu)); > > Also we'd need a double free when the object is freed. Right now > it's handled in the code when the object is not used anymore with a > `g_free(st);`. > I'd have to replace it with > `g_free(st->status); g_free(st);`. Would you suggest doing it place > or by adding a usb_uas_dealloc_status() function? > > --- > > However, I am confused by the use of that variable-lenght field. > I'm looking at what seems the only place where a command is > parsed, in `usb_uas_handle_data`. > > uas_iu iu; > [...] > switch (p->ep->nr) { > case UAS_PIPE_ID_COMMAND: > length = MIN(sizeof(iu), p->iov.size); > usb_packet_copy(p, &iu, length); > [...] > break; > [...] > > It would seem that the copy is limited to at most sizeof(uas_iu), > so even if we had anything in add_cdb[], that wouldn't be copied > here? > > Is this intended? Gerd, do you know?
Hi, > > It would seem that the copy is limited to at most sizeof(uas_iu), > > so even if we had anything in add_cdb[], that wouldn't be copied > > here? > > > > Is this intended? > > Gerd, do you know? Don't remember, it's been a few years ago ... Given that neither add_cdb nor add_cdb_length fields are checked anywhere in the code it is probably save to just comment out the add_cdb field. Should we ever need to look at add_cdb at some point in the future we can figure some better way deal with it, in the simplest case just give it a fixed size based on the needs. take care, Gerd
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c index cec071d96c..5ef3f4fec9 100644 --- a/hw/usb/dev-uas.c +++ b/hw/usb/dev-uas.c @@ -154,9 +154,9 @@ struct UASRequest { struct UASStatus { uint32_t stream; - uas_iu status; uint32_t length; QTAILQ_ENTRY(UASStatus) next; + uas_iu status; }; /* --------------------------------------------------------------------- */
The UASStatus data structure has a variable sized field inside of type uas_iu, that however is not placed at the end of the data structure. This placement triggers a warning with clang 11, and while not a bug right now, (the status is never a uas_iu_command, which is the variable-sized case), it could become one in the future. ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] uas_iu status; ^ 1 error generated. Fix this by moving uas_iu at the end of the struct Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> --- hw/usb/dev-uas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)