| 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 |
> 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(-)