| 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: <202405131203.F7B97F5E38@keescook>
Date: Mon, 13 May 2024 12:43:37 -0700
From: Kees Cook <keescook@...omium.org>
To: Dan Carpenter <dan.carpenter@...aro.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
On Sat, May 11, 2024 at 07:19:49PM +0300, Dan Carpenter wrote:
> 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?
That was the plan, yes. Based on current feedback, we'll be taking an
even more incremental approach, though.
> 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.
Agreed on all counts. :)
> 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?
Sure, I will answer this from the perspective of the original proposal
(enabling signed-integer-overflow, unsigned-integer-overflow,
signed-integer-implicit-truncation, and
unsigned-integer-implicit-truncation).
>
> 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?
Do we _want_ it to overflow? This seems like a case where we don't? I
would expect this to need:
int intval;
if (check_mul_overflow(1000, mA, &intval))
return -EINVAL;
val.intval = intval;
>
> 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;
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) ?
> 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)
>
> 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 }
Again, this doesn't look like we want it overflowing. "idx" isn't
reported correctly. It can just use "idx": that's what's being examined
in the "if" statement.
> 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;
Again, we don't want it wrapping. The whole loop needs to be expanded to
be readable, and bounds checking needs to be performed.
>
>
> 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. (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;
>
> 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;
We don't want this to wrap, yes? I don't know what is expected when regsize == 0,
but ignoring that, I'd expect:
if (check_add_overflow(r[0].start, p->regsize - 1, &r[0].end))
error...
>
>
> 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)
>
> 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.
Either fix the check or:
u32 __wraps divider = ...
>
> 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.
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).
>
> 2266
> 2267 mutex_lock(&data->update_lock);
>
> regards,
> dan carpenter
-Kees
--
Kees Cook
Powered by blists - more mailing lists