Re: [bug report] staging: gpib: Add GPIB common core driver

[Dave Penkler <dpenkler@gmail.com>]

On Fri, Oct 18, 2024 at 09:32:22AM +0300, Dan Carpenter wrote:
> Hello Dave Penkler,
> 
> Commit 9dde4559e939 ("staging: gpib: Add GPIB common core driver")
> from Sep 18, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	drivers/staging/gpib/common/gpib_os.c:541 dvrsp()
> 	warn: no lower bound on 'sad' rl='s32min-30'
> 
> drivers/staging/gpib/common/gpib_os.c
>     525 int dvrsp(gpib_board_t *board, unsigned int pad, int sad,
>     526           unsigned int usec_timeout, uint8_t *result)
>     527 {
>     528         int status = ibstatus(board);
>     529         int retval;
>     530 
>     531         if ((status & CIC) == 0) {
>     532                 pr_err("gpib: not CIC during serial poll\n");
>     533                 return -1;
>     534         }
>     535 
>     536         if (pad > gpib_addr_max || sad > gpib_addr_max) {
> 
> sad comes from the user in serial_poll_ioctl() and it can be any int.
> 
>     537                 pr_err("gpib: bad address for serial poll");
>     538                 return -1;
>     539         }
>     540 
> --> 541         retval = serial_poll_single(board, pad, sad, usec_timeout, result);
> 
> Passing a negative doesn't do anything harmful and it looks like probably it's
> intentional to mean there is no "sad".  But instead of saying that "any negative
> is valid" it would be better to say that "-1" means there is no sad.
> 
> 	if (pad > gpib_addr_max ||
> 	    (sad < -1 || sad > gpib_addr_max)) {
> 		...
> 
> Presumably the user space code is already passing -1 as the no sad value but I
> don't maybe it's passing INT_MIN or something weird.  This is an API change.
> We're allowed to change the API so long as we don't break userspace.  In
> staging, we have a bit more leeway to break userspace as well honestly.
> 
>     542         if (io_timed_out(board))
>     543                 retval = -ETIMEDOUT;
>     544 
>     545         return retval;
>     546 }
> 
> regards,
> dan carpenter
Hi Dan,
Catching up with the backlog.

Unfortunately the user space library uses a weird convention for secondary
addresses.

A secondary address of 0 is specified by adding 96 to it. This was inherited
from the National Instruments API. The library subtracts 96 from the passedi
 value when calling the driver. When the user passes a zero address it 
means no secondary address which corresponds to a value of -96 in the
driver API.

I have changed the user space library to set the sad to -1 when a zero
address is passed and will submit the patch to add the check for the 
lower bound.
cheers,
-Dave