| Message ID | 20240109145121.8850-1-rand.sec96@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: Fix NULL pointer dereference in npcm_i2c_reg_slave | expand |
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?
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 words Wolfram Sang "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. 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? > >
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(-)