[PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[Sergey Shtylyov]

At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
length' field of 'struct ehci_qtd'.  In order not to mix the *signed* and
*unsigned* values let's make that variable and the function's result 'u16'
as qTD's maximum length is a 15-bit quantity anyway...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo.

 drivers/usb/host/ehci-q.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: usb/drivers/usb/host/ehci-q.c
===================================================================
--- usb.orig/drivers/usb/host/ehci-q.c
+++ usb/drivers/usb/host/ehci-q.c
@@ -33,12 +33,13 @@
 
 /* fill a qtd, returning how much of the buffer we were able to queue up */
 
-static int
+static u16
 qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
 		  size_t len, int token, int maxpacket)
 {
-	int	i, count;
 	u64	addr = buf;
+	u16	count;
+	int	i;
 
 	/* one buffer entry per 4K ... first might be short or unaligned */
 	qtd->hw_buf[0] = cpu_to_hc32(ehci, (u32)addr);

RE: [PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[David Laight]

From: Sergey Shtylyov
> Sent: 16 February 2022 20:19
> 
> At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
> length' field of 'struct ehci_qtd'.  In order not to mix the *signed* and
> *unsigned* values let's make that variable and the function's result 'u16'
> as qTD's maximum length is a 15-bit quantity anyway...

Except that you really don't want to be doing arithmetic on sub-register
sized values.
On everything except x86 the compiler will have to add instructions
to mask the value to 16 bits (unless its logic can detect that overflow
can never happen).

There is a similar problem with parameters and return values.
They need masking one side of the call (or maybe both).

> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.

Which clearly doesn't understand the implications of its reports.

	David

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

Re: [PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[Alan Stern]

On Wed, Feb 16, 2022 at 10:33:15PM +0000, David Laight wrote:
> From: Sergey Shtylyov
> > Sent: 16 February 2022 20:19
> > 
> > At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
> > length' field of 'struct ehci_qtd'.  In order not to mix the *signed* and
> > *unsigned* values let's make that variable and the function's result 'u16'
> > as qTD's maximum length is a 15-bit quantity anyway...
> 
> Except that you really don't want to be doing arithmetic on sub-register
> sized values.
> On everything except x86 the compiler will have to add instructions
> to mask the value to 16 bits (unless its logic can detect that overflow
> can never happen).
> 
> There is a similar problem with parameters and return values.
> They need masking one side of the call (or maybe both).
> 
> > Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> > analysis tool.
> 
> Which clearly doesn't understand the implications of its reports.
> 
> 	David

Agreed.  It would be acceptable to change the types to "unsigned int", 
but there's no reason to make them "u16".

In general, the only situation where a size should be smaller than the 
native register size is when you're defining fields in a structure or 
union, or doing memory-mapped I/O (which often involves the same 
thing).

Alan Stern

Re: [PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[Sergey Shtylyov]

Hello!

On 2/17/22 1:33 AM, David Laight wrote:

>> At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
>> length' field of 'struct ehci_qtd'.  In order not to mix the *signed* and
>> *unsigned* values let's make that variable and the function's result 'u16'
>> as qTD's maximum length is a 15-bit quantity anyway...
> 
> Except that you really don't want to be doing arithmetic on sub-register
> sized values.

    So using/returning *unsigned int* instead should be fine?

> On everything except x86 the compiler will have to add instructions
> to mask the value to 16 bits (unless its logic can detect that overflow
> can never happen).

   Yeah, I've only looked at the code produced by x86 gcc, should have tried
e.g. an ARM toolchain as well...

> There is a similar problem with parameters and return values.
> They need masking one side of the call (or maybe both).
> 
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
> 
> Which clearly doesn't understand the implications of its reports.

   The reports are most probably correct (SVACE actually complains about assigning
an *int* variable to 'size_t' field), it's my interpretation which might be
at fault here... :-)

> 	David

MBR, Sergey

RE: [PATCH] usb: host: ehci-q: make qtd_fill() return 'u16'

[David Laight]

From: Sergey Shtylyov
> Sent: 17 February 2022 09:20
...
> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> >> analysis tool.
> >
> > Which clearly doesn't understand the implications of its reports.
> 
>    The reports are most probably correct (SVACE actually complains about assigning
> an *int* variable to 'size_t' field), it's my interpretation which might be
> at fault here... :-)

Which is absolutely fine provided the domain of the value
fits in both types.

There is diddly-squit point reporting errors on assignments
unless the domains of the values can be tracked.

	David

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