On Tue, Apr 04, 2017 at 01:21:33PM +0300, Dan Carpenter wrote:
> 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
This was an awesome find, I totally got this wrong when I did the
patch, I missed the bug you found completely. I re-read the smatch
docs (smatch/Documentation/). I still did not figure out what command
you used to produce this output, my guess is it is either one of the
scripts in smatch_scritps/ or else some combination of options to
kchecker? There are some options to kchecker that are not in the usage
string, is this by design?
While reading the docs I (trivially) converted them to rst format,
would you like a patch sent or is this not important?
I'm going to cc you on v2 of the patch series. I've not done that
before when you have reviewed patches, apologies if that was rude. (In
case you do not want to be cc'd please tell me).
thanks,
Tobin.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
