| Message ID | 20190329111104.17223-11-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc set of fixes for warnings under GCC 9 | expand |
On 29/03/2019 12.11, Daniel P. Berrangé wrote: > The 'filename' field in ObjectInfo struct is declared as a > zero length array of uint16_t. Accessing it is equivalent > to taking the address of the field, and taking the address > of fields in a packed struct causes unaligned pointer > warnings: > > hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’: > hw/usb/dev-mtp.c:1712:36: warning: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Waddress-of-packed-member] > 1712 | dataset->filename); > | ~~~~~~~^~~~~~~~~~ > > The warning is in fact correct because the 'filename' > field is preceeded by a uint8_t field which causes it > to have bad alignment. > > Using pointer arithmetic instead of accessing the zero > length array field directly avoids the compiler warning > but doesn't ultimately fix the bad alignment. Fixing > that probably requires allocating a new array of > uint16_t in the heap & then memcpy() the data before > accessing the array elements. If we are really using an unaligned pointer here, this code will crash on Sparc host machines (and maybe some others). Thus I'd prefer if you could rather fix the misalignment issue here instead of papering over the compiler warning. Thomas
On Fri, Mar 29, 2019 at 12:25:54PM +0100, Thomas Huth wrote: > On 29/03/2019 12.11, Daniel P. Berrangé wrote: > > The 'filename' field in ObjectInfo struct is declared as a > > zero length array of uint16_t. Accessing it is equivalent > > to taking the address of the field, and taking the address > > of fields in a packed struct causes unaligned pointer > > warnings: > > > > hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’: > > hw/usb/dev-mtp.c:1712:36: warning: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Waddress-of-packed-member] > > 1712 | dataset->filename); > > | ~~~~~~~^~~~~~~~~~ > > > > The warning is in fact correct because the 'filename' > > field is preceeded by a uint8_t field which causes it > > to have bad alignment. > > > > Using pointer arithmetic instead of accessing the zero > > length array field directly avoids the compiler warning > > but doesn't ultimately fix the bad alignment. Fixing > > that probably requires allocating a new array of > > uint16_t in the heap & then memcpy() the data before > > accessing the array elements. > > If we are really using an unaligned pointer here, this code will crash > on Sparc host machines (and maybe some others). Thus I'd prefer if you > could rather fix the misalignment issue here instead of papering over > the compiler warning. Yeah, I wasn't sure if uint16 mis-alignment would cause problems or not in practice, so left this comment in the commit message. If you expect this really will crash, then I guess we have to go the memcpy to temporary buffer route. Regards, Daniel
On Fri, 29 Mar 2019 at 11:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > The 'filename' field in ObjectInfo struct is declared as a > zero length array of uint16_t. Accessing it is equivalent > to taking the address of the field, and taking the address > of fields in a packed struct causes unaligned pointer > warnings: > > hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’: > hw/usb/dev-mtp.c:1712:36: warning: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Waddress-of-packed-member] > 1712 | dataset->filename); > | ~~~~~~~^~~~~~~~~~ This one's come up before -- see http://patchwork.ozlabs.org/patch/1049654/ and my comments on it. I think that utf16_to_str() should take a byte array and use the appropriate lduw_*_p() function to read from it, and that we need to think more carefully about endianness and about the "malicious short buffer" case. thanks -- PMM
On Fri, Mar 29, 2019 at 12:04:45PM +0000, Peter Maydell wrote: > On Fri, 29 Mar 2019 at 11:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > The 'filename' field in ObjectInfo struct is declared as a > > zero length array of uint16_t. Accessing it is equivalent > > to taking the address of the field, and taking the address > > of fields in a packed struct causes unaligned pointer > > warnings: > > > > hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’: > > hw/usb/dev-mtp.c:1712:36: warning: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Waddress-of-packed-member] > > 1712 | dataset->filename); > > | ~~~~~~~^~~~~~~~~~ > > This one's come up before -- see > http://patchwork.ozlabs.org/patch/1049654/ > and my comments on it. I think that utf16_to_str() > should take a byte array and use the appropriate > lduw_*_p() function to read from it, and that we > need to think more carefully about endianness and > about the "malicious short buffer" case. Yeah this code is even more of a disaster than i realized. This filename handling is probably CVE worthy. Regards, Daniel
On Fri, 29 Mar 2019 at 15:27, Daniel P. Berrangé <berrange@redhat.com> wrote: > Yeah this code is even more of a disaster than i realized. This filename > handling is probably CVE worthy. My subjective impression is that hw/usb/dev-mtp.c has also been a fertile source of Coverity scan issues; if anybody with an understanding of the relevant bit of the USB spec has the time to do a whole-file code review that might be worthwhile. thanks -- PMM
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 5343449663..9ddcfbe7a6 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -225,7 +225,7 @@ typedef struct { uint16_t assoc_type; uint32_t assoc_desc; uint32_t seq_no; /*unused*/ - uint8_t length; /*part of filename field*/ + uint8_t length; /*length of filename field*/ uint16_t filename[0]; char date_created[0]; /*unused*/ char date_modified[0]; /*unused*/ @@ -1703,13 +1703,15 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen) MTPObject *o; MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle); uint32_t next_handle = s->next_handle; + uint16_t *wfilename = (uint16_t *)(d->data + + offsetof(ObjectInfo, filename)); assert(!s->write_pending); assert(p != NULL); filename = utf16_to_str(MIN(dataset->length, dlen - offsetof(ObjectInfo, filename)), - dataset->filename); + wfilename); if (strchr(filename, '/')) { usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans,
The 'filename' field in ObjectInfo struct is declared as a zero length array of uint16_t. Accessing it is equivalent to taking the address of the field, and taking the address of fields in a packed struct causes unaligned pointer warnings: hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’: hw/usb/dev-mtp.c:1712:36: warning: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Waddress-of-packed-member] 1712 | dataset->filename); | ~~~~~~~^~~~~~~~~~ The warning is in fact correct because the 'filename' field is preceeded by a uint8_t field which causes it to have bad alignment. Using pointer arithmetic instead of accessing the zero length array field directly avoids the compiler warning but doesn't ultimately fix the bad alignment. Fixing that probably requires allocating a new array of uint16_t in the heap & then memcpy() the data before accessing the array elements. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/usb/dev-mtp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)