linux-hardening - RE: [RFC] Mitigating unexpected arithmetic overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d95664b24ec94436b6cf906fefcc0b4f@AcuMS.aculab.com>
Date: Sat, 18 May 2024 15:39:41 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Dan Carpenter' <dan.carpenter@...aro.org>, Kees Cook
	<keescook@...omium.org>
CC: Linus Torvalds <torvalds@...ux-foundation.org>, Justin Stitt
	<justinstitt@...gle.com>, Peter Zijlstra <peterz@...radead.org>, Mark Rutland
	<mark.rutland@....com>, "linux-hardening@...r.kernel.org"
	<linux-hardening@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>
Subject: RE: [RFC] Mitigating unexpected arithmetic overflow

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)