| Message ID | 20240830034640.7049-2-kfting@nuvoton.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/7] i2c: npcm: restore slave addresses array length | expand |
Hi Tyrone, On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > The original design supports 10 slave addresses although only 2 please remember that the "slave" term has been replaced by the "target" term. I will change it when applying the patch. > addresses are required for current implementation. > > Restore the npcm_i2caddr array length to fix the smatch warning. > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") I don't think the Fixes tag is necessary here. This change is primarily addressing a static analyzer warning. While some cases come close to a buffer overflow, it couldn’t have occurred in practice since we don't actually have the devices listed in npcm_i2caddr[]. > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > --- > drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > index 2fe68615942e..bbcb4d6668ce 100644 > --- a/drivers/i2c/busses/i2c-npcm7xx.c > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > @@ -136,11 +136,13 @@ enum i2c_addr { > * Since the addr regs are sprinkled all over the address space, > * use this array to get the address or each register. > */ > -#define I2C_NUM_OWN_ADDR 2 > +#define I2C_NUM_OWN_ADDR 10 > #define I2C_NUM_OWN_ADDR_SUPPORTED 2 > > static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > - NPCM_I2CADDR1, NPCM_I2CADDR2, > + NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4, > + NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8, > + NPCM_I2CADDR9, NPCM_I2CADDR10, Looks a bit hacky, but serves the purpose. The core issue in "npcm_i2c_slave_enable()" is the lack of an upper boundary check, which could potentially lead to a buffer overflow. In practice, we rely on the assumption that these addresses don’t exist in the real world. An easier fix could have been: @@ -629,7 +629,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type, if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) dev_err(bus->dev, "try to enable more than 2 SA not supported\n"); - if (addr_type >= I2C_ARP_ADDR) + if (addr_type > I2C_SLAVE_ADDR2) return -EFAULT; /* Set and enable the address */ But yours is a bit more robust, so that I'm going to take this patch. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi > }; > #endif > > -- > 2.34.1 >
Hi Tyrone, On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > The original design supports 10 slave addresses although only 2 > addresses are required for current implementation. > > Restore the npcm_i2caddr array length to fix the smatch warning. > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> your email used in From: is different that your e-mail used the SoB. Is this done in purpose? If so I will keep it as it is, no problem for me, otherwise I can fix it while applying it. Andi
Hi Andi: Thank you for your review. Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:24寫道： > > Hi Tyrone, > > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > > The original design supports 10 slave addresses although only 2 > > please remember that the "slave" term has been replaced by the > "target" term. I will change it when applying the patch. > Thank you for your reminder. I'll change the term ever since. > > addresses are required for current implementation. > > > > Restore the npcm_i2caddr array length to fix the smatch warning. > > > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > > I don't think the Fixes tag is necessary here. This change is > primarily addressing a static analyzer warning. While some cases > come close to a buffer overflow, it couldn’t have occurred in > practice since we don't actually have the devices listed in > npcm_i2caddr[]. > Understood. I'll remove the Fixes tag. > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > --- > > drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > > index 2fe68615942e..bbcb4d6668ce 100644 > > --- a/drivers/i2c/busses/i2c-npcm7xx.c > > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > > @@ -136,11 +136,13 @@ enum i2c_addr { > > * Since the addr regs are sprinkled all over the address space, > > * use this array to get the address or each register. > > */ > > -#define I2C_NUM_OWN_ADDR 2 > > +#define I2C_NUM_OWN_ADDR 10 > > #define I2C_NUM_OWN_ADDR_SUPPORTED 2 > > > > static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > > - NPCM_I2CADDR1, NPCM_I2CADDR2, > > + NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4, > > + NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8, > > + NPCM_I2CADDR9, NPCM_I2CADDR10, > > Looks a bit hacky, but serves the purpose. > > The core issue in "npcm_i2c_slave_enable()" is the lack of an > upper boundary check, which could potentially lead to a buffer > overflow. In practice, we rely on the assumption that these > addresses don’t exist in the real world. > > An easier fix could have been: > > @@ -629,7 +629,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type, > if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) > dev_err(bus->dev, "try to enable more than 2 SA not supported\n"); > > - if (addr_type >= I2C_ARP_ADDR) > + if (addr_type > I2C_SLAVE_ADDR2) > return -EFAULT; > > /* Set and enable the address */ > > But yours is a bit more robust, so that I'm going to take this > patch. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > > Thanks, > Andi > > > }; > > #endif > > > > -- > > 2.34.1 > > Thank you. Regards, Tyrone
Hi Andy: Thank you for your comments. Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:36寫道： > > Hi Tyrone, > > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > > The original design supports 10 slave addresses although only 2 > > addresses are required for current implementation. > > > > Restore the npcm_i2caddr array length to fix the smatch warning. > > > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > your email used in From: is different that your e-mail used the > SoB. Is this done in purpose? If so I will keep it as it is, no > problem for me, otherwise I can fix it while applying it. > I'll add the option "--from kfting@nuvoton.com", same as the patch author's email while using the tool "git send-email" in the next patch set. > Andi Have a nice day. Thank you. Regards, Tyrone
Hi Tyrone, On Fri, Sep 06, 2024 at 10:28:30AM GMT, Tyrone Ting wrote: > Hi Andy: > > Thank you for your comments. > > Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:36寫道： > > > > Hi Tyrone, > > > > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > > > The original design supports 10 slave addresses although only 2 > > > addresses are required for current implementation. > > > > > > Restore the npcm_i2caddr array length to fix the smatch warning. > > > > > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > > > your email used in From: is different that your e-mail used the > > SoB. Is this done in purpose? If so I will keep it as it is, no > > problem for me, otherwise I can fix it while applying it. > > > > I'll add the option "--from kfting@nuvoton.com", same as the patch > author's email while using the tool "git send-email" > in the next patch set. don't worry, I will apply this patch number '1' because it's independent from the rest of the series. I will do all the changes you agreed with me. When you resend this series you don't need to include this patch, just rebase on top of i2c/i2c-hostp[*]. Thanks, Andi [*] git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git > > Andi > > Have a nice day. > > Thank you. > > Regards, > Tyrone
Hi Tyrone, On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > The original design supports 10 slave addresses although only 2 > addresses are required for current implementation. > > Restore the npcm_i2caddr array length to fix the smatch warning. > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> with the changes we agreed, I merged just this first patch in i2c/i2c-host. Thanks, Andi
Hi Andi: Thank you for your prompt response and help. Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 下午3:05寫道： > > Hi Tyrone, > > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > > The original design supports 10 slave addresses although only 2 > > addresses are required for current implementation. > > > > Restore the npcm_i2caddr array length to fix the smatch warning. > > > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > with the changes we agreed, I merged just this first patch in > i2c/i2c-host. > > Thanks, > Andi Have a nice day. Regards, Tyrone
On Thu, Sep 05, 2024 at 11:36:45PM +0200, Andi Shyti wrote: > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote: > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". > > The original design supports 10 slave addresses although only 2 > > addresses are required for current implementation. > > > > Restore the npcm_i2caddr array length to fix the smatch warning. > > > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > your email used in From: is different that your e-mail used the > SoB. Is this done in purpose? If so I will keep it as it is, no > problem for me, otherwise I can fix it while applying it. IIRC Linux Next has the respective check and it will become your problem :-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index 2fe68615942e..bbcb4d6668ce 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -136,11 +136,13 @@ enum i2c_addr { * Since the addr regs are sprinkled all over the address space, * use this array to get the address or each register. */ -#define I2C_NUM_OWN_ADDR 2 +#define I2C_NUM_OWN_ADDR 10 #define I2C_NUM_OWN_ADDR_SUPPORTED 2 static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { - NPCM_I2CADDR1, NPCM_I2CADDR2, + NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4, + NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8, + NPCM_I2CADDR9, NPCM_I2CADDR10, }; #endif
The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9". The original design supports 10 slave addresses although only 2 addresses are required for current implementation. Restore the npcm_i2caddr array length to fix the smatch warning. Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/ Signed-off-by: Tyrone Ting <kfting@nuvoton.com> --- drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)