| Message ID | 7f2e3194-c897-7ffd-756e-8a9c93d652cd@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | usb: host: ehci-q: make qtd_fill() return 'u16' | expand |
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)
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
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
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)
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);
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(-)