| 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
| ||
|
Message-ID: <039d54d6-8aa2-4e5b-829b-69002cff35d3@moroto.mountain>
Date: Sat, 11 May 2024 19:19:49 +0300
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
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
Powered by blists - more mailing lists