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: Sat, 11 May 2024 19:19:49 +0300 [thread overview]
Message-ID: <039d54d6-8aa2-4e5b-829b-69002cff35d3@moroto.mountain> (raw)
In-Reply-To: <202404291502.612E0A10@keescook>
I'm pretty sure we've tried using a sanitizer before and it had too many
false positives. So your solution is to annotate all the false
positives?
There are two main issue which make integer overflows complicated from
a static analysis perspective. 1) Places where it's intentional or
we don't care. 2) Places where the integer overflow is harmless for
one reason or another. Btw, integer overflows are a lot more common on
32 bit CPUs because obviously it's easier to overflow 4 billion than
whatever number U64_MAX is.
Here are is a sample of ten integer overflows that the user can trigger.
Maybe pick a couple and walk us through how they would be annotated?
drivers/usb/dwc3/gadget.c:3064 dwc3_gadget_vbus_draw() warn: potential integer overflow from user '1000 * mA'
3052 static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
3053 {
3054 struct dwc3 *dwc = gadget_to_dwc(g);
3055 union power_supply_propval val = {0};
3056 int ret;
3057
3058 if (dwc->usb2_phy)
3059 return usb_phy_set_power(dwc->usb2_phy, mA);
3060
3061 if (!dwc->usb_psy)
3062 return -EOPNOTSUPP;
3063
3064 val.intval = 1000 * mA;
^^^^^^^^^^
mA comes from the user and we only know that it's a multiple of 2.
So here we would annotate that val.intval can store integer overflows?
Or we'd annotate mA?
3065 ret = power_supply_set_property(dwc->usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val);
3066
3067 return ret;
3068 }
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;
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.
1995
1996 urb = usb_alloc_urb(packets, GFP_KERNEL);
1997 if (!urb)
1998 return urb;
drivers/usb/storage/uas.c:324 uas_stat_cmplt() warn: potential integer overflow from user 'idx + 1'
322 idx = be16_to_cpup(&iu->tag) - 1;
The "- 1" makes this UINT_MAX
323 if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
324 dev_err(&urb->dev->dev,
325 "stat urb: no pending cmd for uas-tag %d\n", idx + 1);
^^^^^^^
Harmless integer overflow in printk.
326 goto out;
327 }
drivers/mtd/parsers/tplink_safeloader.c:101 mtd_parser_tplink_safeloader_parse() warn: potential integer overflow from user 'bytes + 1'
97 for (idx = 0, offset = TPLINK_SAFELOADER_DATA_OFFSET;
98 idx < TPLINK_SAFELOADER_MAX_PARTS &&
99 sscanf(buf + offset, "partition %64s base 0x%llx size 0x%llx%zn\n",
100 name, &parts[idx].offset, &parts[idx].size, &bytes) == 3;
I think this buffer comes from the partition table?
101 idx++, offset += bytes + 1) {
^^^^^^^^^
102 parts[idx].name = kstrdup(name, GFP_KERNEL);
103 if (!parts[idx].name) {
104 err = -ENOMEM;
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 }
drivers/char/ipmi/ipmi_plat_data.c:70 ipmi_platform_add() warn: potential integer overflow from user 'r[0]->start + p->regsize'
69 r[0].start = p->addr;
70 r[0].end = r[0].start + p->regsize - 1;
^^^^^^^^^^^^^^^^^^^^^^^
I think this is root only so it's safe? Or it could be a false
positive.
71 r[0].name = "IPMI Address 1";
72 r[0].flags = flags;
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.
486 break;
drivers/i2c/busses/i2c-bcm2835.c:93 clk_bcm2835_i2c_calc_divider() warn: potential integer overflow from user 'parent_rate + rate'
90 static int clk_bcm2835_i2c_calc_divider(unsigned long rate,
91 unsigned long parent_rate)
92 {
93 u32 divider = DIV_ROUND_UP(parent_rate, rate);
94
95 /*
96 * Per the datasheet, the register is always interpreted as an even
97 * number, by rounding down. In other words, the LSB is ignored. So,
98 * if the LSB is set, increment the divider to avoid any issue.
99 */
100 if (divider & 1)
101 divider++;
102 if ((divider < BCM2835_I2C_CDIV_MIN) ||
103 (divider > BCM2835_I2C_CDIV_MAX))
104 return -EINVAL;
Again, math first then check the result later. Integer overflow is
basically harmless.
105
106 return divider;
107 }
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.
2266
2267 mutex_lock(&data->update_lock);
regards,
dan carpenter
next prev parent reply other threads:[~2024-05-11 16:19 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 [this message]
2024-05-13 19:43 ` Kees Cook
2024-05-14 8:45 ` Dan Carpenter
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=039d54d6-8aa2-4e5b-829b-69002cff35d3@moroto.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).