| 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
| ||
|
Date: Tue, 14 May 2024 10:45:19 +0200
From: Dan Carpenter <dan.carpenter@...aro.org>
To: 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-kernel@...r.kernel.org,
llvm@...ts.linux.dev
Subject: Re: [RFC] Mitigating unexpected arithmetic overflow
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) ?
>
roundup() has an integer overflow in it.
> > drivers/usb/misc/usbtest.c:1994 iso_alloc_urb() warn: potential integer overflow from user 'bytes + maxp'
> > 1974 static struct urb *iso_alloc_urb(
> > 1975 struct usb_device *udev,
> > 1976 int pipe,
> > 1977 struct usb_endpoint_descriptor *desc,
> > 1978 long bytes,
> > 1979 unsigned offset
> > 1980 )
> > 1981 {
> > 1982 struct urb *urb;
> > 1983 unsigned i, maxp, packets;
> > 1984
> > 1985 if (bytes < 0 || !desc)
> > 1986 return NULL;
> > 1987
> > 1988 maxp = usb_endpoint_maxp(desc);
> > 1989 if (udev->speed >= USB_SPEED_SUPER)
> > 1990 maxp *= ss_isoc_get_packet_num(udev, pipe);
> > 1991 else
> > 1992 maxp *= usb_endpoint_maxp_mult(desc);
> > 1993
> > 1994 packets = DIV_ROUND_UP(bytes, maxp);
> > ^^^^^
> > The issue here is on a 32 bit system when bytes is INT_MAX.
>
> All of these examples seem to be cases that should be mitigated. The
> earlier check should be expanded:
>
> if (bytes < 0 || bytes >= INT_MAX || !desc)
This doesn't work on 32bit systems.
>
> >
> > 1995
> > 1996 urb = usb_alloc_urb(packets, GFP_KERNEL);
> > 1997 if (!urb)
> > 1998 return urb;
> >
> >
> > drivers/char/ppdev.c:344 pp_set_timeout() warn: potential integer overflow from user 'tv_sec * 100'
> > 343 static int pp_set_timeout(struct pardevice *pdev, long tv_sec, int tv_usec)
> > 344 {
> > 345 long to_jiffies;
> > 346
> > 347 if ((tv_sec < 0) || (tv_usec < 0))
> > 348 return -EINVAL;
> > 349
> > 350 to_jiffies = usecs_to_jiffies(tv_usec);
> > ^^^^^^^
> >
> > 351 to_jiffies += tv_sec * HZ;
> > ^^^^^^^^^^^
> > Both of these can overflow
> >
> > 352 if (to_jiffies <= 0)
> > ^^^^^^^^^^^^^^^
> > But they're checked here.
> >
> > 353 return -EINVAL;
> > 354
> > 355 pdev->timeout = to_jiffies;
> > 356 return 0;
> > 357 }
>
> This doesn't look like a wrapping-desired case either, but just for fun,
> let's assume we want it.
The original programmer assumed it would wrap so it was intentional/desired.
> (And why are any of these signed?) Annotation
> is added to the variables:
>
> static int pp_set_timeout(struct pardevice *pdev, long __wraps tv_sec, int __wraps tv_usec)
> {
> long __wraps to_jiffies;
Sure.
> > drivers/i2c/i2c-dev.c:485 i2cdev_ioctl() warn: potential integer overflow from user (local copy) 'arg * 10'
> > 478 case I2C_TIMEOUT:
> > 479 if (arg > INT_MAX)
> > 480 return -EINVAL;
> > 481
> > 482 /* For historical reasons, user-space sets the timeout
> > 483 * value in units of 10 ms.
> > 484 */
> > 485 client->adapter->timeout = msecs_to_jiffies(arg * 10);
> > ^^^^^^^^^
> > This can overflow and then the msecs_to_jiffies() conversion also has
> > an integer overflow in it.
>
> If we want these wrapping:
>
> unsigned long __wraps timeout = arg * 10;
> client->adapter->timeout = msecs_to_jiffies(timeout);
>
> and:
>
> static inline unsigned long _msecs_to_jiffies(const unsigned int __wraps m)
>
>
Here we don't care about wrapping, but I wouldn't go so far as to say it
was intentional... I guess this silences the warning.
> > drivers/hwmon/nct6775-core.c:2265 store_temp_offset() warn: potential integer overflow from user '__x + (__d / 2)'
> > 2251 static ssize_t
> > 2252 store_temp_offset(struct device *dev, struct device_attribute *attr,
> > 2253 const char *buf, size_t count)
> > 2254 {
> > 2255 struct nct6775_data *data = dev_get_drvdata(dev);
> > 2256 struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> > 2257 int nr = sattr->index;
> > 2258 long val;
> > 2259 int err;
> > 2260
> > 2261 err = kstrtol(buf, 10, &val);
> > 2262 if (err < 0)
> > 2263 return err;
> > 2264
> > 2265 val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
> > ^^^^^^^^^^
> > Overflow and then clamp.
>
> Looks like DIV_ROUND_CLOSEST() needs to be made sane and use more modern
> helpers (it appears to be doing open-coded is_signed(), wants
> check_*_overflow(), etc).
>
Sounds good.
regards,
dan carpenter
Powered by blists - more mailing lists