From: Dan Carpenter <dan.carpenter@linaro.org>
To: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Justin Stitt <justinstitt@google.com>,
Peter Zijlstra <peterz@infradead.org>,
Mark Rutland <mark.rutland@arm.com>,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev
Subject: Re: [RFC] Mitigating unexpected arithmetic overflow
Date: Tue, 14 May 2024 10:45:19 +0200 [thread overview]
Message-ID: <0480d602-3ad7-4f8e-a480-9860d56eab30@suswa.mountain> (raw)
In-Reply-To: <202405131203.F7B97F5E38@keescook>
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
next prev parent reply other threads:[~2024-05-14 8:45 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 23:27 [RFC] Mitigating unexpected arithmetic overflow Kees Cook
2024-05-08 12:22 ` David Laight
2024-05-08 23:43 ` Kees Cook
2024-05-08 17:52 ` Linus Torvalds
2024-05-08 19:44 ` Kees Cook
2024-05-08 20:07 ` Linus Torvalds
2024-05-08 22:54 ` Kees Cook
2024-05-08 23:47 ` Linus Torvalds
2024-05-09 0:06 ` Linus Torvalds
2024-05-09 0:23 ` Linus Torvalds
2024-05-09 6:11 ` Kees Cook
2024-05-09 14:08 ` Theodore Ts'o
2024-05-09 15:38 ` Linus Torvalds
2024-05-09 17:54 ` Al Viro
2024-05-09 18:08 ` Linus Torvalds
2024-05-09 18:39 ` Linus Torvalds
2024-05-09 18:48 ` Al Viro
2024-05-09 19:15 ` Linus Torvalds
2024-05-09 19:28 ` Al Viro
2024-05-09 21:06 ` David Laight
2024-05-18 5:11 ` Matthew Wilcox
2024-05-09 21:23 ` David Laight
2024-05-12 8:03 ` Martin Uecker
2024-05-12 16:09 ` Linus Torvalds
2024-05-12 19:29 ` Martin Uecker
2024-05-13 18:34 ` Kees Cook
2024-05-15 7:36 ` Peter Zijlstra
2024-05-15 17:12 ` Justin Stitt
2024-05-16 7:45 ` Peter Zijlstra
2024-05-16 13:30 ` Kees Cook
2024-05-16 14:09 ` Peter Zijlstra
2024-05-16 19:48 ` Justin Stitt
2024-05-16 20:07 ` Kees Cook
2024-05-16 20:51 ` Theodore Ts'o
2024-05-17 21:15 ` Kees Cook
2024-05-18 2:51 ` Theodore Ts'o
2024-05-17 22:04 ` Fangrui Song
2024-05-18 13:08 ` David Laight
2024-05-15 7:57 ` Peter Zijlstra
2024-05-17 7:45 ` Jonas Oberhauser
2024-05-11 16:19 ` Dan Carpenter
2024-05-13 19:43 ` Kees Cook
2024-05-14 8:45 ` Dan Carpenter [this message]
2024-05-18 15:39 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0480d602-3ad7-4f8e-a480-9860d56eab30@suswa.mountain \
--to=dan.carpenter@linaro.org \
--cc=justinstitt@google.com \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=mark.rutland@arm.com \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).