| Message ID | 20240323063852.665639-1-shum.sdl@nppct.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iphase: Adding a null pointer check | expand |
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@nppct.ru> > --- > 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@mail.gmail.com/ [2] https://lore.kernel.org/all/20240322154337.4f78858c@kernel.org/ > + 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?
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 @@ -192,6 +192,11 @@ static u16 get_desc (IADEV *dev, struct ia_vcc *iavcc) { i++; continue; } + 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) {
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@nppct.ru> --- drivers/atm/iphase.c | 5 +++++ 1 file changed, 5 insertions(+)