| Message ID | 20230418142918.70510-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] b43legacy: Add checking for null for ssb_get_devtypedata(dev) | expand |
On Tue, 18 Apr 2023 07:29:18 -0700 Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote: > Since second call of ssb_get_devtypedata() may fail as well as the > first one, the NULL return value in 'wl' will be later dereferenced in > calls to b43legacy_one_core_attach() and schedule_work(). No, the second call can't fail, because b43legacy_wireless_init() will always initialize it before returning 0. > a/drivers/net/wireless/broadcom/b43legacy/main.c +++ > b/drivers/net/wireless/broadcom/b43legacy/main.c @@ -3857,7 +3857,11 > @@ static int b43legacy_probe(struct ssb_device *dev, if (err) > goto out; > wl = ssb_get_devtypedata(dev); > - B43legacy_WARN_ON(!wl); > + if (!wl) { > + B43legacy_WARN_ON(!wl); > + err = -ENODEV; > + goto out; And the 'goto out' would be the wrong error recovery path, too. > + } > } > err = b43legacy_one_core_attach(dev, wl); > if (err) Nack. Please drop this patch. The code is correct as-is.
On 4/18/23 09:29, Nikita Zhandarovich wrote: > Since second call of ssb_get_devtypedata() may fail as well as the > first one, the NULL return value in 'wl' will be later dereferenced in > calls to b43legacy_one_core_attach() and schedule_work(). > > Instead of merely warning about this failure with > B43legacy_WARN_ON(), properly return with error to avoid any further > NULL pointer dereferences. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Fixes: 75388acd0cd8 ("[B43LEGACY]: add mac80211-based driver for legacy BCM43xx devices") > Co-developed-by: Natalia Petrova <n.petrova@fintech.ru> > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > v2: fix issues with overlooked null-ptr-dereferences pointed out by > Simon Horman <simon.horman@corigine.com> > Link: https://lore.kernel.org/all/Y+eb9mZntfe6rO3v@corigine.com/ > > drivers/net/wireless/broadcom/b43legacy/main.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c b/drivers/net/wireless/broadcom/b43legacy/main.c > index 760136638a95..5a706dd0b1a4 100644 > --- a/drivers/net/wireless/broadcom/b43legacy/main.c > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c > @@ -3857,7 +3857,11 @@ static int b43legacy_probe(struct ssb_device *dev, > if (err) > goto out; > wl = ssb_get_devtypedata(dev); > - B43legacy_WARN_ON(!wl); > + if (!wl) { > + B43legacy_WARN_ON(!wl); > + err = -ENODEV; > + goto out; > + } > } > err = b43legacy_one_core_attach(dev, wl); > if (err) I do not recall seeing v1. One additional nitpick: The latest convention would have the subject as "wifi: b43legacy:...". Kalle may be able to fix this on merging, but it not, a v3 might be required. Otherwise, the patch is good. Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net> Thanks, Larry
On Fri, 21 Apr 2023 17:14:18 -0500 Larry Finger <Larry.Finger@lwfinger.net> wrote: > > (err) goto out; > > wl = ssb_get_devtypedata(dev); > > - B43legacy_WARN_ON(!wl); > > + if (!wl) { > > + B43legacy_WARN_ON(!wl); > > + err = -ENODEV; > > + goto out; > > + } > > } > > err = b43legacy_one_core_attach(dev, wl); > > if (err) > > I do not recall seeing v1. One additional nitpick: The latest > convention would have the subject as "wifi: b43legacy:...". Kalle may > be able to fix this on merging, but it not, a v3 might be required. > Otherwise, the patch is good. > > Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net> No, it's not good. It's wrong. I already replied to it. wl can never be NULL here and the goto-out path is wrong (if there was a chance for it to trigger). Please drop this patch, Kalle.
Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote: > Since second call of ssb_get_devtypedata() may fail as well as the > first one, the NULL return value in 'wl' will be later dereferenced in > calls to b43legacy_one_core_attach() and schedule_work(). > > Instead of merely warning about this failure with > B43legacy_WARN_ON(), properly return with error to avoid any further > NULL pointer dereferences. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Fixes: 75388acd0cd8 ("[B43LEGACY]: add mac80211-based driver for legacy BCM43xx devices") > Co-developed-by: Natalia Petrova <n.petrova@fintech.ru> > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net> Dropped per Michael's request. Patch set to Rejected.
diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c b/drivers/net/wireless/broadcom/b43legacy/main.c index 760136638a95..5a706dd0b1a4 100644 --- a/drivers/net/wireless/broadcom/b43legacy/main.c +++ b/drivers/net/wireless/broadcom/b43legacy/main.c @@ -3857,7 +3857,11 @@ static int b43legacy_probe(struct ssb_device *dev, if (err) goto out; wl = ssb_get_devtypedata(dev); - B43legacy_WARN_ON(!wl); + if (!wl) { + B43legacy_WARN_ON(!wl); + err = -ENODEV; + goto out; + } } err = b43legacy_one_core_attach(dev, wl); if (err)
Since second call of ssb_get_devtypedata() may fail as well as the first one, the NULL return value in 'wl' will be later dereferenced in calls to b43legacy_one_core_attach() and schedule_work(). Instead of merely warning about this failure with B43legacy_WARN_ON(), properly return with error to avoid any further NULL pointer dereferences. Found by Linux Verification Center (linuxtesting.org) with static analysis tool SVACE. Fixes: 75388acd0cd8 ("[B43LEGACY]: add mac80211-based driver for legacy BCM43xx devices") Co-developed-by: Natalia Petrova <n.petrova@fintech.ru> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> --- v2: fix issues with overlooked null-ptr-dereferences pointed out by Simon Horman <simon.horman@corigine.com> Link: https://lore.kernel.org/all/Y+eb9mZntfe6rO3v@corigine.com/ drivers/net/wireless/broadcom/b43legacy/main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)