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

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

[Dan Carpenter]

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

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

[Dave Penkler]

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