| Message ID | 20240326160342.3588864-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | USB: gadget: dummy_hcd: switch char * to u8 * | expand |
On Tue, 26 Mar 2024 at 09:03, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > The function handle_control_request() casts the urb buffer to a char *, > and then treats it like a unsigned char buffer when assigning data to > it. On some architectures, "char" is really signed, so let's just > properly set this pointer to a u8 to take away any potential problems as > that's what is really wanted here. [..] > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Well, I assume this goes back to the discussions almost two years ago that then just led us to use '-funsigned-char' for the kernel. So the patch is still correct, but it's not like it's strictly necessary. I have no idea how this re-surfaced now. Linus
On Tue, Mar 26, 2024 at 05:03:43PM +0100, Greg Kroah-Hartman wrote: > The function handle_control_request() casts the urb buffer to a char *, > and then treats it like a unsigned char buffer when assigning data to > it. On some architectures, "char" is really signed, so let's just > properly set this pointer to a u8 to take away any potential problems as > that's what is really wanted here. Yes, it certainly ought to be u8 rather than char. > Document that we are only using the lower 8 bits for the devstatus > variable (only 7 are currently used), as the cast from 16 to 8 is not > obvious. It wouldn't hurt to change the code so that it copies all 16 bits. I think it was originally done the way it is now because that was easier, not because there was any significance to the 8-bit/16-bit alteration. > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Jakob Koschel <jakobkoschel@gmail.com> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/gadget/udc/dummy_hcd.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c > index 0953e1b5c030..1139fc8c03f0 100644 > --- a/drivers/usb/gadget/udc/dummy_hcd.c > +++ b/drivers/usb/gadget/udc/dummy_hcd.c > @@ -1739,13 +1739,13 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb, > if (setup->bRequestType == Dev_InRequest > || setup->bRequestType == Intf_InRequest > || setup->bRequestType == Ep_InRequest) { > - char *buf; > + u8 *buf; > /* > - * device: remote wakeup, selfpowered > + * device: remote wakeup, selfpowered, LTM, U1, or U2 Also B_HNP_ENABLE, A_HNP_SUPPORT, A_ALT_HNP_SUPPORT. Not sure it's really worthwhile to list all these things in a comment. Also, it would be good to reorder the comment lines so that they match the code: endpoint first, then device, then interface. Alan Stern > * interface: nothing > * endpoint: halt > */ > - buf = (char *)urb->transfer_buffer; > + buf = urb->transfer_buffer; > if (urb->transfer_buffer_length > 0) { > if (setup->bRequestType == Ep_InRequest) { > ep2 = find_endpoint(dum, w_index); > @@ -1754,11 +1754,12 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb, > break; > } > buf[0] = ep2->halted; > - } else if (setup->bRequestType == > - Dev_InRequest) { > + } else if (setup->bRequestType == Dev_InRequest) { > + /* Only take the lower 8 bits */ > buf[0] = (u8)dum->devstatus; > - } else > + } else { > buf[0] = 0; > + } > } > if (urb->transfer_buffer_length > 1) > buf[1] = 0; > -- > 2.44.0 >
On Tue, Mar 26, 2024 at 09:16:12AM -0700, Linus Torvalds wrote: > On Tue, 26 Mar 2024 at 09:03, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > The function handle_control_request() casts the urb buffer to a char *, > > and then treats it like a unsigned char buffer when assigning data to > > it. On some architectures, "char" is really signed, so let's just > > properly set this pointer to a u8 to take away any potential problems as > > that's what is really wanted here. > [..] > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > > Well, I assume this goes back to the discussions almost two years ago > that then just led us to use '-funsigned-char' for the kernel. Yes. > So the patch is still correct, but it's not like it's strictly > necessary. I have no idea how this re-surfaced now. It was in my really old patch queue and I noticed it had never been updated or merged, so I dug it up and fixed it based on Alan's review. thanks, greg k-h
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index 0953e1b5c030..1139fc8c03f0 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -1739,13 +1739,13 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb, if (setup->bRequestType == Dev_InRequest || setup->bRequestType == Intf_InRequest || setup->bRequestType == Ep_InRequest) { - char *buf; + u8 *buf; /* - * device: remote wakeup, selfpowered + * device: remote wakeup, selfpowered, LTM, U1, or U2 * interface: nothing * endpoint: halt */ - buf = (char *)urb->transfer_buffer; + buf = urb->transfer_buffer; if (urb->transfer_buffer_length > 0) { if (setup->bRequestType == Ep_InRequest) { ep2 = find_endpoint(dum, w_index); @@ -1754,11 +1754,12 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb, break; } buf[0] = ep2->halted; - } else if (setup->bRequestType == - Dev_InRequest) { + } else if (setup->bRequestType == Dev_InRequest) { + /* Only take the lower 8 bits */ buf[0] = (u8)dum->devstatus; - } else + } else { buf[0] = 0; + } } if (urb->transfer_buffer_length > 1) buf[1] = 0;
The function handle_control_request() casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here. Document that we are only using the lower 8 bits for the devstatus variable (only 7 are currently used), as the cast from 16 to 8 is not obvious. Cc: Felipe Balbi <balbi@kernel.org> Cc: Jakob Koschel <jakobkoschel@gmail.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Alan Stern <stern@rowland.harvard.edu> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/gadget/udc/dummy_hcd.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)