[linux-next:master 1342/8914] drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

[linux-next:master 1342/8914] drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

[Dan Carpenter]

Hi Tyrone,

First bad commit (maybe != root cause):

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head:   f2528c29385819a84480cacef4886b049761e2c5
commit: bbc38ed53a02a759d8e5c01e834eca49304a2315 [1342/8914] i2c: npcm: Support NPCM845
config: microblaze-randconfig-m031-20220706 (https://download.01.org/0day-ci/archive/20220711/202207110811.lWIJpo4l-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

vim +/npcm_i2caddr +639 drivers/i2c/busses/i2c-npcm7xx.c

f54736925a4f83 Tali Perry 2020-05-27  607  static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
f54736925a4f83 Tali Perry 2020-05-27  608  				 u8 addr, bool enable)
f54736925a4f83 Tali Perry 2020-05-27  609  {
f54736925a4f83 Tali Perry 2020-05-27  610  	u8 i2cctl1;
f54736925a4f83 Tali Perry 2020-05-27  611  	u8 i2cctl3;
f54736925a4f83 Tali Perry 2020-05-27  612  	u8 sa_reg;
f54736925a4f83 Tali Perry 2020-05-27  613  
f54736925a4f83 Tali Perry 2020-05-27  614  	sa_reg = (addr & 0x7F) | FIELD_PREP(NPCM_I2CADDR_SAEN, enable);
f54736925a4f83 Tali Perry 2020-05-27  615  	if (addr_type == I2C_GC_ADDR) {
f54736925a4f83 Tali Perry 2020-05-27  616  		i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
f54736925a4f83 Tali Perry 2020-05-27  617  		if (enable)
f54736925a4f83 Tali Perry 2020-05-27  618  			i2cctl1 |= NPCM_I2CCTL1_GCMEN;
f54736925a4f83 Tali Perry 2020-05-27  619  		else
f54736925a4f83 Tali Perry 2020-05-27  620  			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
f54736925a4f83 Tali Perry 2020-05-27  621  		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
f54736925a4f83 Tali Perry 2020-05-27  622  		return 0;
47d506d1a28fd1 Tali Perry 2022-05-25  623  	} else if (addr_type == I2C_ARP_ADDR) {
f54736925a4f83 Tali Perry 2020-05-27  624  		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
f54736925a4f83 Tali Perry 2020-05-27  625  		if (enable)
f54736925a4f83 Tali Perry 2020-05-27  626  			i2cctl3 |= I2CCTL3_ARPMEN;
f54736925a4f83 Tali Perry 2020-05-27  627  		else
f54736925a4f83 Tali Perry 2020-05-27  628  			i2cctl3 &= ~I2CCTL3_ARPMEN;
f54736925a4f83 Tali Perry 2020-05-27  629  		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
f54736925a4f83 Tali Perry 2020-05-27  630  		return 0;
f54736925a4f83 Tali Perry 2020-05-27  631  	}
47d506d1a28fd1 Tali Perry 2022-05-25  632  	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
47d506d1a28fd1 Tali Perry 2022-05-25  633  		dev_err(bus->dev, "try to enable more than 2 SA not supported\n");

This prints an error message for values 2-10, but allows 0-1,11,12.
Maybe the intention here was to return -EINVAL?  It seldom makes sense
to print an error and then go forward with an out of bounds access.

47d506d1a28fd1 Tali Perry 2022-05-25  634  
f54736925a4f83 Tali Perry 2020-05-27  635  	if (addr_type >= I2C_ARP_ADDR)
                                                                 ^^^^^^^^^^^^
This is addr_type >= 11 so Smatch complains that npcm_i2caddr[] only has
two elements.  My personal Smatch run with the cross function DB says
that addr_type is always 0 so it doesn't complain.

However, one rule of kernel style is that we do not allow stub code and
also to a human reader this code really does look buggy...  :/

f54736925a4f83 Tali Perry 2020-05-27  636  		return -EFAULT;
47d506d1a28fd1 Tali Perry 2022-05-25  637  
f54736925a4f83 Tali Perry 2020-05-27  638  	/* Set and enable the address */
f54736925a4f83 Tali Perry 2020-05-27 @639  	iowrite8(sa_reg, bus->reg + npcm_i2caddr[addr_type]);
f54736925a4f83 Tali Perry 2020-05-27  640  	npcm_i2c_slave_int_enable(bus, enable);
47d506d1a28fd1 Tali Perry 2022-05-25  641  
f54736925a4f83 Tali Perry 2020-05-27  642  	return 0;
f54736925a4f83 Tali Perry 2020-05-27  643  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

RE: [linux-next:master 1342/8914] drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

[KFTING]

Hi Dan:

Thank you for your findings.

It's under discussion.

Thank you.

Regards,
Tyrone


-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com>
Sent: Tuesday, July 12, 2022 10:01 PM
To: kbuild@lists.01.org; CS20 KFTing <KFTING@nuvoton.com>
Cc: lkp@intel.com; kbuild-all@lists.01.org; Linux Memory Management List <linux-mm@kvack.org>; Wolfram Sang <wsa-dev@sang-engineering.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: [linux-next:master 1342/8914] drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

Hi Tyrone,

First bad commit (maybe != root cause):

tree:   https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fnext%2Flinux-next.git&amp;data=05%7C01%7Ckfting%40nuvoton.com%7Cdba8ad969d8b45249e8508da640f0f12%7Ca3f24931d4034b4a94f17d83ac638e07%7C0%7C0%7C637932747253333274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=noT2aCRrctRlLER1OEP40Q%2FGKYBjArSfJ9JsSodCmmo%3D&amp;reserved=0 master
head:   f2528c29385819a84480cacef4886b049761e2c5
commit: bbc38ed53a02a759d8e5c01e834eca49304a2315 [1342/8914] i2c: npcm: Support NPCM845
config: microblaze-randconfig-m031-20220706 (https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdownload.01.org%2F0day-ci%2Farchive%2F20220711%2F202207110811.lWIJpo4l-lkp%40intel.com%2Fconfig&amp;data=05%7C01%7Ckfting%40nuvoton.com%7Cdba8ad969d8b45249e8508da640f0f12%7Ca3f24931d4034b4a94f17d83ac638e07%7C0%7C0%7C637932747253333274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Rvg3gcFwhuBYfnbBb5%2Fq04ohZH66pIv5cLq6KKE%2BfPg%3D&amp;reserved=0)
compiler: microblaze-linux-gcc (GCC) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/i2c/busses/i2c-npcm7xx.c:639 npcm_i2c_slave_enable() error: buffer overflow 'npcm_i2caddr' 2 <= 9

vim +/npcm_i2caddr +639 drivers/i2c/busses/i2c-npcm7xx.c

f54736925a4f83 Tali Perry 2020-05-27  607  static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
f54736925a4f83 Tali Perry 2020-05-27  608                                u8 addr, bool enable)
f54736925a4f83 Tali Perry 2020-05-27  609  {
f54736925a4f83 Tali Perry 2020-05-27  610       u8 i2cctl1;
f54736925a4f83 Tali Perry 2020-05-27  611       u8 i2cctl3;
f54736925a4f83 Tali Perry 2020-05-27  612       u8 sa_reg;
f54736925a4f83 Tali Perry 2020-05-27  613
f54736925a4f83 Tali Perry 2020-05-27  614       sa_reg = (addr & 0x7F) | FIELD_PREP(NPCM_I2CADDR_SAEN, enable);
f54736925a4f83 Tali Perry 2020-05-27  615       if (addr_type == I2C_GC_ADDR) {
f54736925a4f83 Tali Perry 2020-05-27  616               i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
f54736925a4f83 Tali Perry 2020-05-27  617               if (enable)
f54736925a4f83 Tali Perry 2020-05-27  618                       i2cctl1 |= NPCM_I2CCTL1_GCMEN;
f54736925a4f83 Tali Perry 2020-05-27  619               else
f54736925a4f83 Tali Perry 2020-05-27  620                       i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
f54736925a4f83 Tali Perry 2020-05-27  621               iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
f54736925a4f83 Tali Perry 2020-05-27  622               return 0;
47d506d1a28fd1 Tali Perry 2022-05-25  623       } else if (addr_type == I2C_ARP_ADDR) {
f54736925a4f83 Tali Perry 2020-05-27  624               i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
f54736925a4f83 Tali Perry 2020-05-27  625               if (enable)
f54736925a4f83 Tali Perry 2020-05-27  626                       i2cctl3 |= I2CCTL3_ARPMEN;
f54736925a4f83 Tali Perry 2020-05-27  627               else
f54736925a4f83 Tali Perry 2020-05-27  628                       i2cctl3 &= ~I2CCTL3_ARPMEN;
f54736925a4f83 Tali Perry 2020-05-27  629               iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
f54736925a4f83 Tali Perry 2020-05-27  630               return 0;
f54736925a4f83 Tali Perry 2020-05-27  631       }
47d506d1a28fd1 Tali Perry 2022-05-25  632       if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
47d506d1a28fd1 Tali Perry 2022-05-25  633               dev_err(bus->dev, "try to enable more than 2 SA not supported\n");

This prints an error message for values 2-10, but allows 0-1,11,12.
Maybe the intention here was to return -EINVAL?  It seldom makes sense to print an error and then go forward with an out of bounds access.

47d506d1a28fd1 Tali Perry 2022-05-25  634
f54736925a4f83 Tali Perry 2020-05-27  635       if (addr_type >= I2C_ARP_ADDR)
                                                                 ^^^^^^^^^^^^ This is addr_type >= 11 so Smatch complains that npcm_i2caddr[] only has two elements.  My personal Smatch run with the cross function DB says that addr_type is always 0 so it doesn't complain.

However, one rule of kernel style is that we do not allow stub code and also to a human reader this code really does look buggy...  :/

f54736925a4f83 Tali Perry 2020-05-27  636               return -EFAULT;
47d506d1a28fd1 Tali Perry 2022-05-25  637
f54736925a4f83 Tali Perry 2020-05-27  638       /* Set and enable the address */
f54736925a4f83 Tali Perry 2020-05-27 @639       iowrite8(sa_reg, bus->reg + npcm_i2caddr[addr_type]);
f54736925a4f83 Tali Perry 2020-05-27  640       npcm_i2c_slave_int_enable(bus, enable);
47d506d1a28fd1 Tali Perry 2022-05-25  641
f54736925a4f83 Tali Perry 2020-05-27  642       return 0;
f54736925a4f83 Tali Perry 2020-05-27  643  }

--
0-DAY CI Kernel Test Service
https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2F01.org%2Flkp&amp;data=05%7C01%7Ckfting%40nuvoton.com%7Cdba8ad969d8b45249e8508da640f0f12%7Ca3f24931d4034b4a94f17d83ac638e07%7C0%7C0%7C637932747253333274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=rWK79iKTAeZDNJjfoymuDF7zrvRB0TcnaBF%2FOwX4xFw%3D&amp;reserved=0

________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.