[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.

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(-)

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;
-- 
2.44.0

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

[Linus Torvalds]

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

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

[Greg Kroah-Hartman]

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

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

[Alan Stern]

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
> 

[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