Re: [PATCH] iphase: Adding a null pointer check

[Simon Horman]

On Sat, Mar 23, 2024 at 09:38:52AM +0300, Andrey Shumilin wrote:
> The pointer <dev->desc_tbl[i].iavcc> is dereferenced on line 195.
> Further in the code, it is checked for null on line 204.
> It is proposed to add a check before dereferencing the pointer.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Andrey Shumilin <shum.sdl@xxxxxxxx>
> ---
>  drivers/atm/iphase.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c
> index 324148686953..596422fbfacc 100644
> --- a/drivers/atm/iphase.c
> +++ b/drivers/atm/iphase.c

1.

The line immediately above the provided patch is:

	if (!dev->desc_tbl[i].timestamp) {

> @@ -192,6 +192,11 @@ static u16 get_desc (IADEV *dev, struct ia_vcc *iavcc) {
>             i++;
>             continue;
>          }

So the dereference will not be hit if .timestamp is zero.
And, lightly examining the code, it seems likely to me
that if .iavcc is NULL then .timestamp is always 0.

As Eric and Jakub have stated in relation to other patches [1][2],
it really would be best to provide a clear explanation of how
the problem can manifest.

[1] https://lore.kernel.org/all/CANn89iK1SO32Zggz5fh4J=NmrVW5RjkdbxJ+-ULP8ysmKXLGvg@xxxxxxxxxxxxxx/
[2] https://lore.kernel.org/all/20240322154337.4f78858c@xxxxxxxxxx/

> +       if (!(iavcc_r = dev->desc_tbl[i].iavcc)) {
> +	   printk("Fatal err, desc table vcc or skb is NULL\n");
> +	   i++;
> +	   continue;
> +	}
>          ltimeout = dev->desc_tbl[i].iavcc->ltimeout; 
>          delta = jiffies - dev->desc_tbl[i].timestamp;
>          if (delta >= ltimeout) {

2.

A little further down is a check for NULL as described in the patch
description:

           if (!dev->desc_tbl[i].txskb || !(iavcc_r = dev->desc_tbl[i].iavcc))
              printk("Fatal err, desc table vcc or skb is NULL\n");

Assuming the proposed check should be added (which I do not believe
is the case) then I believe that the "skb" portion of the message
that has been copied from the existing check relates to checking
txskb. So either .txskb should also be checked or the "skb" portion of the
message should be dropped.

3.

After a quick scan it seems to me that all changes to this file since the
beginning of git history relate to tree-wide changes, clean-ups, addressing
problems flagged by static analysis, and so on.

I do not see a single commit to this file relating to real work on this driver,
f.e. addressing a problem observed by someone using the driver.
If so (please check!) perhaps we should discuss removing it?

-- 
pw-bot: changes-requested