[PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Greg Kroah-Hartman]

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.

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>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/gadget/udc/dummy_hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index 899ac9f9c279..774781968e55 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -1740,13 +1740,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
 			 * 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);
-- 
2.38.1

Re: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Linus Torvalds]

On Thu, Oct 20, 2022 at 11:44 PM 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.

I think you might as well also remove the cast that was always a bit odd:

                buf[0] = (u8)dum->devstatus;

although maybe it's intentional ("look, ma, I'm truncating this
value") because 'devstatus' is a 'u16' type?

I suspect a comment would be more readable than an odd cast that
doesn't actually change anything (since the assignment does it
anyway).

Or maybe people wrote it that way on purpose, and used that variable
name on purpose.

Because 'dum' is Swedish for 'stupid', and maybe there's a coded
message in that driver?

                  Linus

RE: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[David Laight]

From: Linus Torvalds
> Sent: 21 October 2022 18:31
...
> I think you might as well also remove the cast that was always a bit odd:
> 
>                 buf[0] = (u8)dum->devstatus;
...
> I suspect a comment would be more readable than an odd cast that
> doesn't actually change anything (since the assignment does it
> anyway).

I've seen a compiler generate an '& 0xff' for the cast before
storing the low byte.
Don't even think about what that compiler would generated for:
		buf[0] = (u8)(dum->devstatus & 0xff);

The code is much easier to read as just an assignment :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Greg Kroah-Hartman]

On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote:
> On Thu, Oct 20, 2022 at 11:44 PM 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.
> 
> I think you might as well also remove the cast that was always a bit odd:
> 
>                 buf[0] = (u8)dum->devstatus;
> 
> although maybe it's intentional ("look, ma, I'm truncating this
> value") because 'devstatus' is a 'u16' type?

(adding Alan as he's the owner of this file now)

Yes, devstatus is a u16 as that's what the USB spec says it should be,
but so far only 7 of the lower bits have been used.  I guess to do this
properly we should also copy the upper 8 bits in to buf[1], eventhough
in reality it's only ever going to be 0x00 for now.

Although if we ever do get another 2 status bits defined, this code will
break so we probably should do that too.

I'll go do that for a v2 of this and properly comment it.

> I suspect a comment would be more readable than an odd cast that
> doesn't actually change anything (since the assignment does it
> anyway).
> 
> Or maybe people wrote it that way on purpose, and used that variable
> name on purpose.
> 
> Because 'dum' is Swedish for 'stupid', and maybe there's a coded
> message in that driver?

That whole driver is called "dummy" as it's a "fake" driver, not a
"stupid" one :)

thanks,

greg k-h

Re: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Greg Kroah-Hartman]

On Sun, Oct 23, 2022 at 05:53:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote:
> > On Thu, Oct 20, 2022 at 11:44 PM 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.
> > 
> > I think you might as well also remove the cast that was always a bit odd:
> > 
> >                 buf[0] = (u8)dum->devstatus;
> > 
> > although maybe it's intentional ("look, ma, I'm truncating this
> > value") because 'devstatus' is a 'u16' type?
> 
> (adding Alan as he's the owner of this file now)
> 
> Yes, devstatus is a u16 as that's what the USB spec says it should be,
> but so far only 7 of the lower bits have been used.  I guess to do this
> properly we should also copy the upper 8 bits in to buf[1], eventhough
> in reality it's only ever going to be 0x00 for now.

Along these lines, do we really not have a predefined macro/inline
function that does:
	(value >> 8)
to give you the "high byte" of a 16bit value?  I keep seeing people
write their own macros for this in staging drivers, but I just
attributed that to them not using the correct in-kernel macro, but I
can't seem to find anything at the moment to do this (same with "give me
just the lower 8 bits of a 16bit value").

Am I just blind?

It's not like it's complex or tricky stuff, I just thought we had
something in bits.h or bitops.h or the like.  Oh well...

thanks,

greg k-h

Re: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Linus Torvalds]

On Sun, Oct 23, 2022 at 9:04 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Along these lines, do we really not have a predefined macro/inline
> function that does:
>         (value >> 8)
> to give you the "high byte" of a 16bit value?

No macros like that. And honestly, why would you want a macro that is
more complicated than the operation itself?

But it sounds like what you actually want is just

     put_unaligned_le16(dum->devstatus, buf);

which does both bytes correctly (and turns into a plain 16-bit store
on sane architectures)..

               Linus

Re: [PATCH] USB: gadget: dummy_hcd: switch char * to u8 *

[Alan Stern]

On Sun, Oct 23, 2022 at 05:53:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote:
> > On Thu, Oct 20, 2022 at 11:44 PM 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.
> > 
> > I think you might as well also remove the cast that was always a bit odd:
> > 
> >                 buf[0] = (u8)dum->devstatus;
> > 
> > although maybe it's intentional ("look, ma, I'm truncating this
> > value") because 'devstatus' is a 'u16' type?
> 
> (adding Alan as he's the owner of this file now)
> 
> Yes, devstatus is a u16 as that's what the USB spec says it should be,
> but so far only 7 of the lower bits have been used.  I guess to do this
> properly we should also copy the upper 8 bits in to buf[1], eventhough
> in reality it's only ever going to be 0x00 for now.

I count only 5 of the bits being used, not 7.  (See Figure 9-4 in 
section 9.4.5 of the USB-3.1 spec.)  Maybe you're thinking of Feature 
flags rather than Status bits?  They do have a lot of overlap.

Dave Brownell wrote that code initially, so we'll never know why he 
included the cast.  My guess is the same as Linus's: It's there to alert 
the reader that a 16-bit value is being shortened to squeeze into an 
8-bit slot.

> Although if we ever do get another 2 status bits defined, this code will
> break so we probably should do that too.
> 
> I'll go do that for a v2 of this and properly comment it.

Note that there's an out-of-date comment line just above this part of 
the code:

		 * device: remote wakeup, selfpowered

Thanks to USB-3 support, the device recipient now defines three more 
bits in devstatus: LTM_ENABLED, U1_ENABLED, and U2_ENABLED.

Alan Stern