> -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Monday, August 12, 2013 1:32 PM > To: Stalley, Sean > Cc: linux-usb@xxxxxxxxxxxxxxx; Sarah Sharp; Ismail, Abdul R; Alan Stern > Subject: Re: [PATCH v3] usb: rh_call_control tbuf potential future overflow fix > > On Mon, Aug 12, 2013 at 11:09:25AM -0700, Sean O. Stalley wrote: > > rh_call_control() contains a buffer, tbuf, which it uses to hold USB > > descriptors. These discriptors are eventually copied into the > > transfer_buffer in the URB. The buffer in the URB is dynamically > > defined and is always large enough to hold the amount of data it > > requests. > > > > tbuf is currently statically allocated on the stack with a size of 15 > > bytes, regardless of the size specified in the URB. > > This patch dynamically allocates tbuf, and ensures that tbuf is at > > least as big as the buffer in the URB. > > > > If an hcd attempts to write a descriptor containing more than > > 15 bytes ( such as the Standard BOS Descriptor for hubs, defined in > > the USB3.0 Spec, section 10.13.1 ) the write would overflow the buffer > > and corrupt the stack. This patch addresses this behavior. > > > > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Sean O. Stalley <sean.stalley@xxxxxxxxx> > > --- > > drivers/usb/core/hcd.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > Applying this patch produces a complier warning that shows the patch is > buggy: > > drivers/usb/core/hcd.c: In function ‘usb_hcd_submit_urb’: I should note that tbuf isn't in function 'usb_hcd_submit_urb', it is in the static function 'rh_call_control'. 'usb_hcd_submit_urb' calls 'rh_urb_enqueue', which calls 'rh_call_control' > drivers/usb/core/hcd.c:704:7: warning: ‘tbuf’ may be used uninitialized in this > function [-Wmaybe-uninitialized] This line is kfree(tbuf); > drivers/usb/core/hcd.c:474:7: note: ‘tbuf’ was declared here I do not see how tbuf could not be initialized by the time it gets to 'kfree'. Unless 'kzalloc' fails, 'kzalloc' and 'kfree' are unconditional and sequential, so I'm not sure how you could get one without the other. All of the goto's in this function are below the 'kzalloc' and above 'kfree', so allocation (and release) cannot be jumped over. > > gcc is correct here, please fix this, and _never_ ignore complier warnings. If I set tbuf to NULL when I declare it, the warning goes away. Is this an acceptable solution? > > thanks, > > greg k-h Thanks, Sean O. Stalley ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
