On Sun, Apr 02, 2017 at 10:18:09AM +1000, Tobin C. Harding wrote:
> Constant is used to allocate memory for a buffer, then buffer is
> filled upto 'size' which is passed as a parameter. If 'size' is bigger
> than the constant then the buffer will overflow. Function has internal
> linkage so this can only happen due to programmer error. BUG_ON() is
> designed for catching these cases. Currently there is only one call
> site and it is correct, adding BUG_ON() will potentially save
> developer time if later changes to the code are incorrect.
>
> Use BUG_ON() to guard buffer write size in function with internal linkage.
Nah. Adding these BUG_ON() calls everywhere is ugly. There are tons
and tons of assumptions like this in the kernel and we'd have to add
thousands of BUG_ON()s to cover everything.
You say the caller is correct but Smatch thinks it's not very safe.
drivers/staging/ks7010/ks7010_sdio.c
739 goto release_host_and_free;
740
741 length = fw_entry->size;
length is an int. Let's imagine that it's negative.
742
743 /* Load Program */
744 n = 0;
745 do {
746 if (length >= ROM_BUFF_SIZE) {
^^^^^^^^^^^^^^^^^^^^^^^
length is negative so it's less than ROM_BUFF_SIZE.
747 size = ROM_BUFF_SIZE;
748 length = length - ROM_BUFF_SIZE;
749 } else {
750 size = length;
size is u32 so it's now a huge size.
751 length = 0;
752 }
753 DPRINTK(4, "size = %d\n", size);
754 if (size == 0)
755 break;
756 memcpy(rom_buf, fw_entry->data + n, size);
757 /* Update write index */
758 offset = n;
759 ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
760 if (ret)
761 goto release_firmware;
762
763 /* Write data */
764 ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
^^^^
We're passing a huge value for size.
Instead of adding a BUG_ON(), let's just make "length" unsigned.
There is a Smatch interface to see all this information but it's a bit
unwieldy. I've got a couple ideas to maybe improve it later this month
and maybe a few years down the line it would also be nice to integrate
this information with the vim so you could highlight every possibly
unsafe array access. Right now Smatch tries to give you a very limited
set of warnings for things which are highly risky, but if it were
integrated with the IDE we could be far more picky about every unsafe
thing.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
