Message ID | 20240109145121.8850-1-rand.sec96@gmail.com |
---|---|
State | Rejected |
Headers | show |
Series | i2c: Fix NULL pointer dereference in npcm_i2c_reg_slave | expand |
On Tue, Jan 9, 2024 at 4:52 PM Rand Deeb <rand.sec96@gmail.com> wrote: > > In the npcm_i2c_reg_slave function, a potential NULL pointer dereference > issue occurs when 'client' is NULL. This patch adds a proper NULL check for > 'client' at the beginning of the function to prevent undefined behavior. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Rand Deeb <rand.sec96@gmail.com> > --- > drivers/i2c/busses/i2c-npcm7xx.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > index c1b679737240..cfabfb50211d 100644 > --- a/drivers/i2c/busses/i2c-npcm7xx.c > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > @@ -1243,13 +1243,14 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) > static int npcm_i2c_reg_slave(struct i2c_client *client) > { > unsigned long lock_flags; > - struct npcm_i2c *bus = i2c_get_adapdata(client->adapter); > - > - bus->slave = client; > + struct npcm_i2c *bus; > > - if (!bus->slave) > + if (!client) > return -EINVAL; > > + bus = i2c_get_adapdata(client->adapter); > + bus->slave = client; > + > if (client->flags & I2C_CLIENT_TEN) > return -EAFNOSUPPORT; > > -- > 2.34.1 > Thanks for the patch! Reviewed-by:tali.perry1@gmail.com
On 24/01/10 10:43AM, Tali Perry wrote: > On Tue, Jan 9, 2024 at 4:52 PM Rand Deeb <rand.sec96@gmail.com> wrote: > > > > In the npcm_i2c_reg_slave function, a potential NULL pointer dereference > > issue occurs when 'client' is NULL. This patch adds a proper NULL check for > > 'client' at the beginning of the function to prevent undefined behavior. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Signed-off-by: Rand Deeb <rand.sec96@gmail.com> > > --- > > drivers/i2c/busses/i2c-npcm7xx.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > > index c1b679737240..cfabfb50211d 100644 > > --- a/drivers/i2c/busses/i2c-npcm7xx.c > > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > > @@ -1243,13 +1243,14 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) > > static int npcm_i2c_reg_slave(struct i2c_client *client) > > { > > unsigned long lock_flags; > > - struct npcm_i2c *bus = i2c_get_adapdata(client->adapter); > > - > > - bus->slave = client; > > + struct npcm_i2c *bus; > > > > - if (!bus->slave) > > + if (!client) > > return -EINVAL; > > > > + bus = i2c_get_adapdata(client->adapter); > > + bus->slave = client; > > + > > if (client->flags & I2C_CLIENT_TEN) > > return -EAFNOSUPPORT; > > > > -- > > 2.34.1 > > > > > Thanks for the patch! > > Reviewed-by:tali.perry1@gmail.com If I'm not missing something, npcm_i2c_reg_slave() is called via a function pointer ->reg_slave here [1]. And seems `client` can't be NULL there. Other drivers implementing ->reg_slave function don't check its argument. Maybe we should just drop `if (!bus->slave)` check? [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-slave.c#L48
> If I'm not missing something, npcm_i2c_reg_slave() is called via a > function pointer ->reg_slave here [1]. And seems `client` can't be NULL > there. Other drivers implementing ->reg_slave function don't check its > argument. Correct, we trust ourselves here. > Maybe we should just drop `if (!bus->slave)` check? Yes.
On 24/02/03 09:44PM, Wolfram Sang wrote: > > > If I'm not missing something, npcm_i2c_reg_slave() is called via a > > function pointer ->reg_slave here [1]. And seems `client` can't be NULL > > there. Other drivers implementing ->reg_slave function don't check its > > argument. > > Correct, we trust ourselves here. > > > Maybe we should just drop `if (!bus->slave)` check? > > Yes. > Okay, thanks for confirmation. Rand, would you like to prepare the patch, please?
On Sun, Feb 4, 2024 at 11:54 AM Fedor Pchelkin <pchelkin@ispras.ru> wrote: > > On 24/02/03 09:44PM, Wolfram Sang wrote: > > > > > If I'm not missing something, npcm_i2c_reg_slave() is called via a > > > function pointer ->reg_slave here [1]. And seems `client` can't be NULL > > > there. Other drivers implementing ->reg_slave function don't check its > > > argument. > > > > Correct, we trust ourselves here. > > > > > Maybe we should just drop `if (!bus->slave)` check? > > > > Yes. > > > > Okay, thanks for confirmation. > > Rand, would you like to prepare the patch, please? > Hi Fedor!, Sure, In fact, there were two scenarios from the beginning, either redundant condition or potential NULL pointer dereference.I relied on the condition to determine the type of issue because I did not find it logical to add a useless condition, but based on the Wolfram Sang words "we trust ourselves here." then the scenario will change to redundant condition, so i'll write a new patch and send it in new thread.
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index c1b679737240..cfabfb50211d 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -1243,13 +1243,14 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) static int npcm_i2c_reg_slave(struct i2c_client *client) { unsigned long lock_flags; - struct npcm_i2c *bus = i2c_get_adapdata(client->adapter); - - bus->slave = client; + struct npcm_i2c *bus; - if (!bus->slave) + if (!client) return -EINVAL; + bus = i2c_get_adapdata(client->adapter); + bus->slave = client; + if (client->flags & I2C_CLIENT_TEN) return -EAFNOSUPPORT;
In the npcm_i2c_reg_slave function, a potential NULL pointer dereference issue occurs when 'client' is NULL. This patch adds a proper NULL check for 'client' at the beginning of the function to prevent undefined behavior. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Rand Deeb <rand.sec96@gmail.com> --- drivers/i2c/busses/i2c-npcm7xx.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)