On Mon, Aug 12, 2013 at 11:16:24PM +0000, Stalley, Sean wrote: > > > > -----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' Odd, the joys of inline functions? Very strange, but the warning is still correct. > > 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'. Look closer... > 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. Look earlier in the function, you could jump to error, which would eventually call kfree(tbuf) with tbuf being undefined. > > 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? Yes, that's the correct solution. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
