[PATCH 03/24] staging: ks7010: add BUG_ON() to catch programmer error

Tobin C. Harding me at tobin.cc
Tue Apr 4 11:59:30 UTC 2017


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.


More information about the devel mailing list