[10/14] usb-mtp: avoid warning about unaligned access to filename
diff mbox series

Message ID 20190329111104.17223-11-berrange@redhat.com
State New
Headers show
Series
  • misc set of fixes for warnings under GCC 9
Related show

Commit Message

Daniel P. Berrangé March 29, 2019, 11:11 a.m. UTC
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(-)

Comments

Thomas Huth March 29, 2019, 11:25 a.m. UTC | #1
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
Daniel P. Berrangé March 29, 2019, 11:58 a.m. UTC | #2
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
Peter Maydell March 29, 2019, 12:04 p.m. UTC | #3
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
Daniel P. Berrangé March 29, 2019, 3:27 p.m. UTC | #4
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
Peter Maydell March 29, 2019, 5:23 p.m. UTC | #5
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

Patch
diff mbox series

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,