lkml.org 
[lkml]   [2024]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC] Mitigating unexpected arithmetic overflow
Date
From: Dan Carpenter
> Sent: 14 May 2024 09:45
>
> Snipped all the bits where you are clearly correct.
>
> On Mon, May 13, 2024 at 12:43:37PM -0700, Kees Cook wrote:
> > > drivers/usb/class/usbtmc.c:852 usbtmc_generic_read() warn: potential integer overflow from user
> 'max_transfer_size + 1'
> > > 842 * wMaxPacketSize – 1) to avoid sending a zero-length
> > > 843 * packet
> > > 844 */
> > > 845 remaining = transfer_size;
> > > 846 if ((max_transfer_size % data->wMaxPacketSize) == 0)
> > > 847 max_transfer_size += (data->wMaxPacketSize - 1);
> > > 848 } else {
> > > 849 /* round down to bufsize to avoid truncated data left */
> > > 850 if (max_transfer_size > bufsize) {
> > > 851 max_transfer_size =
> > > 852 roundup(max_transfer_size + 1 - bufsize,
> > > ^^^^^^^^^^^^^^^^^^^^^
> > > This can overflow. We should make it a rule that all size variables
> > > have to be unsigned long. That would have made this safe on 64 bit
> > > systems.
> > >
> > > 853 bufsize);
> > > 854 }
> > > 855 remaining = max_transfer_size;
> >
> > Again, do we _want_ this to overflow? It looks like not. I'm not sure
> > what this code is trying to do, though. The comment doesn't seem to
> > match the code. Why isn't this just roundup(max_transfer_size, bufsize) ?
> >

Isn't it just max_transfer_size / bufsize * bufsize?

> roundup() has an integer overflow in it.

Which is a generic problem with these 'helpers'.
If the function is open coded any overflow is obvious.
But hide it in a wrapper and it is just 'assumed to work'.
DIV_ROUNDUP(x, y) can be either (x + y - 1)/y or (x - 1)/y + 1.
The first is valid for 0 but can overflow, the second is valid for x != 0.
(Who knows what is expected for negative values!)
In most places one of the pair will always be correct.

Obfuscating the code tend to stop readers (and the kernel code does get some)
spotting things in passing.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
\
 
 \ /
  Last update: 2024-05-18 17:42    [W:0.095 / U:0.520 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site