Good eye, I completely missed the first goto. I'll update the patch and resend it. Thanks Greg!
-Sean > -----Original Message----- > From: Greg Kroah-Hartman [mailto:[email protected]] > Sent: Monday, August 12, 2013 5:28 PM > To: Stalley, Sean > Cc: [email protected]; 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:16:24PM +0000, Stalley, Sean wrote: > > > > > > > -----Original Message----- > > > From: Greg Kroah-Hartman [mailto:[email protected]] > > > Sent: Monday, August 12, 2013 1:32 PM > > > To: Stalley, Sean > > > Cc: [email protected]; 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 <[email protected]> > > > > Signed-off-by: Sean O. Stalley <[email protected]> > > > > --- > > > > 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
