| Message ID | 20250513123754.3041911-1-ruc_gongyuanjun@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] drivers/i2c: fix a potential null pointer dereference | expand |
On Tue, May 13, 2025 at 2:38 PM Yuanjun Gong <ruc_gongyuanjun@163.com> wrote: > Add check to *priv to make sure the input *dev is not NULL, > and to avoid a potential null pointer dereference. > > Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> Why is this a problem? Reading nmk_i2c_probe(): priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; OK so priv is not NULL. amba_set_drvdata(adev, priv); It is unconditionally assigned to the AMBA device. Surely runtime resume isn't called before probe() has commenced successfully? I don't understand when this condition can occur. Also including Theo on this, he is the major industrial user of this driver now. Yours, Linus Walleij
Hello Linus, Yuanjun, On Wed May 14, 2025 at 12:10 AM CEST, Linus Walleij wrote: > On Tue, May 13, 2025 at 2:38 PM Yuanjun Gong <ruc_gongyuanjun@163.com> wrote: > >> Add check to *priv to make sure the input *dev is not NULL, >> and to avoid a potential null pointer dereference. >> >> Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> > > Why is this a problem? Reading nmk_i2c_probe(): > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > OK so priv is not NULL. > > amba_set_drvdata(adev, priv); > > It is unconditionally assigned to the AMBA device. > > Surely runtime resume isn't called before probe() has commenced > successfully? > > I don't understand when this condition can occur. > > Also including Theo on this, he is the major industrial user of this driver now. Thanks for the Cc; I agree with your reading of the code and don't see the issue. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
> > Also including Theo on this, he is the major industrial user of this driver now. > > Thanks for the Cc; I agree with your reading of the code and don't see > the issue. Likely a false positive from static code analysis.
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index d2877e4cc28d..ebc600f3a7c8 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -968,6 +968,9 @@ static int nmk_i2c_runtime_resume(struct device *dev) struct nmk_i2c_dev *priv = amba_get_drvdata(adev); int ret; + if (!priv) + return -EINVAL; + ret = clk_prepare_enable(priv->clk); if (ret) { dev_err(dev, "can't prepare_enable clock\n");
Add check to *priv to make sure the input *dev is not NULL, and to avoid a potential null pointer dereference. Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> --- drivers/i2c/busses/i2c-nomadik.c | 3 +++ 1 file changed, 3 insertions(+)