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
