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.
So do you mean we should we not make the assumptions, or that we should
check input arguments and return an error if assumptions are violated?
I read that BUG_ON() is not liked in many cases, is there any cases
where BUG_ON() is good to use? I tried to read up on it and from what
I could gather this was the use case. Did I miss-read?
> 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);
If you get time could you elaborate on how you called smatch to get
these results please so I can reproduce them locally (or point me at
the manual). I have been using kchecker on all my patches but
obviously I need to do more. It is a nice tool, I would like to be
able to use it more thoroughly.
> ^^^^
> 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.
That would be super cool.
> regards,
> dan carpenter
I will rework this patch and re-submit.
thanks,
Tobin.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
