2002-09-26 02:25:11

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

>>>>+ envp[i++] = scratch;
>>>>+ scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
>>>>+ pdev->vendor, pdev->device) + 1;
>>>And so forth. Use "snprintf" and prevent overrunning those buffers...
>>Hmm? An %04X format is perfectly bounded.

Which was my thought when I originally wrote the code which has been
widely cut'n'pasted. Safe enough at that moment, but it's now become
dangerous to leave that around as a copy/paste coding example.

Those code fragments are now being used in contexts that aren't
as controlled as the original: the code _using_ the buffer is no
longer in charge of allocating it. There's no longer any way to
guarantee that adding the next parameter to the environment isn't
going to overrun the available space.

Except by using "snprintf" and tracking how much space is left.

Easy enough to do, and that's a habit that IMO _everyone_ should
be getting into whenever they program in languages that permit
such buffer overflows.

> Technically, it isn't bounded. The field will expand if the value exceeds
> 4 digits.
> However, these values can't do that. At least not now.
> But, as a good programming practice, snprintf should be used. Heck, PCI
> 3.0 might use 32-bit vendor and device values, instead of 8 bit. So, if
> nothing else, do it as insurance for the future.

And to help ensure that as people continue to copy/paste code from the
core hotpluggable systems, they won't break things when they add the
parameters needed to set up their new subsystem or class, using the
/sbin/hotplug mechanism.

- Dave