diff mbox series

i2c: core-smbus: fix a potential uninitialization bug

Message ID 1525484596-5585-1-git-send-email-wang6495@umn.edu
State Superseded
Headers show
Series i2c: core-smbus: fix a potential uninitialization bug | expand

Commit Message

Wenwen Wang May 5, 2018, 1:43 a.m. UTC 
In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
which are used to save a series of messages, as mentioned in the comment.
According to the value of the variable "size", msgbuf0 is initialized to
various values. In contrast, msgbuf1 is left uninitialized until the
function i2c_transfer() is invoked. However, mgsbuf1 is not always
initialized on all possible execution paths (implementation) of
i2c_transfer(). Thus, it is possible that mgsbuf1 may still be
uninitialized even after the invocation of the function i2c_transfer(),
especially when the return value of ic2_transfer() is not checked properly.
In the following execution, the uninitialized msgbuf1 will be used, such as
for security checks. Since uninitialized values can be random and
arbitrary, this will cause undefined behaviors or even check bypass. For
example, it is expected that if the value of "size" is
I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
value read from msgbuf1 is assigned to data->block[0], which can
potentially lead to invalid block write size, as demonstrated in the error
message.

This patch checks the return value of i2c_transfer() and also initializes
the first byte of msgbuf1 with 0 to avoid undefined behaviors or security
issues.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/i2c/i2c-core-smbus.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

kernel test robot May 5, 2018, 10:15 a.m. UTC | #1 
Hi Wenwen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wenwen-Wang/i2c-core-smbus-fix-a-potential-uninitialization-bug/20180505-164208
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-a0-201817 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//i2c/i2c-core-smbus.c: In function 'i2c_smbus_xfer_emulated':
>> drivers//i2c/i2c-core-smbus.c:347:2: error: 'msgbug1' undeclared (first use in this function)
     msgbug1[0] = 0;
     ^
   drivers//i2c/i2c-core-smbus.c:347:2: note: each undeclared identifier is reported only once for each function it appears in

vim +/msgbug1 +347 drivers//i2c/i2c-core-smbus.c

   310	
   311	/*
   312	 * Simulate a SMBus command using the I2C protocol.
   313	 * No checking of parameters is done!
   314	 */
   315	static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
   316					   unsigned short flags,
   317					   char read_write, u8 command, int size,
   318					   union i2c_smbus_data *data)
   319	{
   320		/*
   321		 * So we need to generate a series of msgs. In the case of writing, we
   322		 * need to use only one message; when reading, we need two. We
   323		 * initialize most things with sane defaults, to keep the code below
   324		 * somewhat simpler.
   325		 */
   326		unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
   327		unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
   328		int num = read_write == I2C_SMBUS_READ ? 2 : 1;
   329		int i;
   330		u8 partial_pec = 0;
   331		int status;
   332		struct i2c_msg msg[2] = {
   333			{
   334				.addr = addr,
   335				.flags = flags,
   336				.len = 1,
   337				.buf = msgbuf0,
   338			}, {
   339				.addr = addr,
   340				.flags = flags | I2C_M_RD,
   341				.len = 0,
   342				.buf = msgbuf1,
   343			},
   344		};
   345	
   346		msgbuf0[0] = command;
 > 347		msgbug1[0] = 0;
   348		switch (size) {
   349		case I2C_SMBUS_QUICK:
   350			msg[0].len = 0;
   351			/* Special case: The read/write field is used as data */
   352			msg[0].flags = flags | (read_write == I2C_SMBUS_READ ?
   353						I2C_M_RD : 0);
   354			num = 1;
   355			break;
   356		case I2C_SMBUS_BYTE:
   357			if (read_write == I2C_SMBUS_READ) {
   358				/* Special case: only a read! */
   359				msg[0].flags = I2C_M_RD | flags;
   360				num = 1;
   361			}
   362			break;
   363		case I2C_SMBUS_BYTE_DATA:
   364			if (read_write == I2C_SMBUS_READ)
   365				msg[1].len = 1;
   366			else {
   367				msg[0].len = 2;
   368				msgbuf0[1] = data->byte;
   369			}
   370			break;
   371		case I2C_SMBUS_WORD_DATA:
   372			if (read_write == I2C_SMBUS_READ)
   373				msg[1].len = 2;
   374			else {
   375				msg[0].len = 3;
   376				msgbuf0[1] = data->word & 0xff;
   377				msgbuf0[2] = data->word >> 8;
   378			}
   379			break;
   380		case I2C_SMBUS_PROC_CALL:
   381			num = 2; /* Special case */
   382			read_write = I2C_SMBUS_READ;
   383			msg[0].len = 3;
   384			msg[1].len = 2;
   385			msgbuf0[1] = data->word & 0xff;
   386			msgbuf0[2] = data->word >> 8;
   387			break;
   388		case I2C_SMBUS_BLOCK_DATA:
   389			if (read_write == I2C_SMBUS_READ) {
   390				msg[1].flags |= I2C_M_RECV_LEN;
   391				msg[1].len = 1; /* block length will be added by
   392						   the underlying bus driver */
   393				i2c_smbus_try_get_dmabuf(&msg[1], 0);
   394			} else {
   395				msg[0].len = data->block[0] + 2;
   396				if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
   397					dev_err(&adapter->dev,
   398						"Invalid block write size %d\n",
   399						data->block[0]);
   400					return -EINVAL;
   401				}
   402	
   403				i2c_smbus_try_get_dmabuf(&msg[0], command);
   404				for (i = 1; i < msg[0].len; i++)
   405					msg[0].buf[i] = data->block[i - 1];
   406			}
   407			break;
   408		case I2C_SMBUS_BLOCK_PROC_CALL:
   409			num = 2; /* Another special case */
   410			read_write = I2C_SMBUS_READ;
   411			if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
   412				dev_err(&adapter->dev,
   413					"Invalid block write size %d\n",
   414					data->block[0]);
   415				return -EINVAL;
   416			}
   417	
   418			msg[0].len = data->block[0] + 2;
   419			i2c_smbus_try_get_dmabuf(&msg[0], command);
   420			for (i = 1; i < msg[0].len; i++)
   421				msg[0].buf[i] = data->block[i - 1];
   422	
   423			msg[1].flags |= I2C_M_RECV_LEN;
   424			msg[1].len = 1; /* block length will be added by
   425					   the underlying bus driver */
   426			i2c_smbus_try_get_dmabuf(&msg[1], 0);
   427			break;
   428		case I2C_SMBUS_I2C_BLOCK_DATA:
   429			if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
   430				dev_err(&adapter->dev, "Invalid block %s size %d\n",
   431					read_write == I2C_SMBUS_READ ? "read" : "write",
   432					data->block[0]);
   433				return -EINVAL;
   434			}
   435	
   436			if (read_write == I2C_SMBUS_READ) {
   437				msg[1].len = data->block[0];
   438				i2c_smbus_try_get_dmabuf(&msg[1], 0);
   439			} else {
   440				msg[0].len = data->block[0] + 1;
   441	
   442				i2c_smbus_try_get_dmabuf(&msg[0], command);
   443				for (i = 1; i <= data->block[0]; i++)
   444					msg[0].buf[i] = data->block[i];
   445			}
   446			break;
   447		default:
   448			dev_err(&adapter->dev, "Unsupported transaction %d\n", size);
   449			return -EOPNOTSUPP;
   450		}
   451	
   452		i = ((flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK
   453					      && size != I2C_SMBUS_I2C_BLOCK_DATA);
   454		if (i) {
   455			/* Compute PEC if first message is a write */
   456			if (!(msg[0].flags & I2C_M_RD)) {
   457				if (num == 1) /* Write only */
   458					i2c_smbus_add_pec(&msg[0]);
   459				else /* Write followed by read */
   460					partial_pec = i2c_smbus_msg_pec(0, &msg[0]);
   461			}
   462			/* Ask for PEC if last message is a read */
   463			if (msg[num-1].flags & I2C_M_RD)
   464				msg[num-1].len++;
   465		}
   466	
   467		status = i2c_transfer(adapter, msg, num);
   468		if (status < 0)
   469			return status;
   470		if (status != num)
   471			return -EIO;
   472	
   473		/* Check PEC if last message is a read */
   474		if (i && (msg[num-1].flags & I2C_M_RD)) {
   475			status = i2c_smbus_check_pec(partial_pec, &msg[num-1]);
   476			if (status < 0)
   477				return status;
   478		}
   479	
   480		if (read_write == I2C_SMBUS_READ)
   481			switch (size) {
   482			case I2C_SMBUS_BYTE:
   483				data->byte = msgbuf0[0];
   484				break;
   485			case I2C_SMBUS_BYTE_DATA:
   486				data->byte = msgbuf1[0];
   487				break;
   488			case I2C_SMBUS_WORD_DATA:
   489			case I2C_SMBUS_PROC_CALL:
   490				data->word = msgbuf1[0] | (msgbuf1[1] << 8);
   491				break;
   492			case I2C_SMBUS_I2C_BLOCK_DATA:
   493				for (i = 0; i < data->block[0]; i++)
   494					data->block[i + 1] = msg[1].buf[i];
   495				break;
   496			case I2C_SMBUS_BLOCK_DATA:
   497			case I2C_SMBUS_BLOCK_PROC_CALL:
   498				for (i = 0; i < msg[1].buf[0] + 1; i++)
   499					data->block[i] = msg[1].buf[i];
   500				break;
   501			}
   502	
   503		if (msg[0].flags & I2C_M_DMA_SAFE)
   504			kfree(msg[0].buf);
   505		if (msg[1].flags & I2C_M_DMA_SAFE)
   506			kfree(msg[1].buf);
   507	
   508		return 0;
   509	}
   510	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Peter Rosin May 5, 2018, 10:28 a.m. UTC | #2 
On 2018-05-05 03:43, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable "size", msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still be
> uninitialized even after the invocation of the function i2c_transfer(),
> especially when the return value of ic2_transfer() is not checked properly.
> In the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of "size" is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch checks the return value of i2c_transfer() and also initializes
> the first byte of msgbuf1 with 0 to avoid undefined behaviors or security
> issues.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/i2c/i2c-core-smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..e8470d5 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  	};
>  
>  	msgbuf0[0] = command;
> +	msgbug1[0] = 0;
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
>  		msg[0].len = 0;
> @@ -466,6 +467,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  	status = i2c_transfer(adapter, msg, num);
>  	if (status < 0)
>  		return status;
> +	if (status != num)
> +		return -EIO;
>  
>  	/* Check PEC if last message is a read */
>  	if (i && (msg[num-1].flags & I2C_M_RD)) {
> 

I think these two hunks should be two separate patches. They address
orthogonal issues...

Cheers,
Peter
kernel test robot May 5, 2018, 11:50 a.m. UTC | #3 
Hi Wenwen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wenwen-Wang/i2c-core-smbus-fix-a-potential-uninitialization-bug/20180505-164208
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: x86_64-randconfig-x013-201817 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/i2c/i2c-core-smbus.c: In function 'i2c_smbus_xfer_emulated':
>> drivers/i2c/i2c-core-smbus.c:347:2: error: 'msgbug1' undeclared (first use in this function); did you mean 'msgbuf1'?
     msgbug1[0] = 0;
     ^~~~~~~
     msgbuf1
   drivers/i2c/i2c-core-smbus.c:347:2: note: each undeclared identifier is reported only once for each function it appears in

vim +347 drivers/i2c/i2c-core-smbus.c

   310	
   311	/*
   312	 * Simulate a SMBus command using the I2C protocol.
   313	 * No checking of parameters is done!
   314	 */
   315	static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
   316					   unsigned short flags,
   317					   char read_write, u8 command, int size,
   318					   union i2c_smbus_data *data)
   319	{
   320		/*
   321		 * So we need to generate a series of msgs. In the case of writing, we
   322		 * need to use only one message; when reading, we need two. We
   323		 * initialize most things with sane defaults, to keep the code below
   324		 * somewhat simpler.
   325		 */
   326		unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
   327		unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
   328		int num = read_write == I2C_SMBUS_READ ? 2 : 1;
   329		int i;
   330		u8 partial_pec = 0;
   331		int status;
   332		struct i2c_msg msg[2] = {
   333			{
   334				.addr = addr,
   335				.flags = flags,
   336				.len = 1,
   337				.buf = msgbuf0,
   338			}, {
   339				.addr = addr,
   340				.flags = flags | I2C_M_RD,
   341				.len = 0,
   342				.buf = msgbuf1,
   343			},
   344		};
   345	
   346		msgbuf0[0] = command;
 > 347		msgbug1[0] = 0;
   348		switch (size) {
   349		case I2C_SMBUS_QUICK:
   350			msg[0].len = 0;
   351			/* Special case: The read/write field is used as data */
   352			msg[0].flags = flags | (read_write == I2C_SMBUS_READ ?
   353						I2C_M_RD : 0);
   354			num = 1;
   355			break;
   356		case I2C_SMBUS_BYTE:
   357			if (read_write == I2C_SMBUS_READ) {
   358				/* Special case: only a read! */
   359				msg[0].flags = I2C_M_RD | flags;
   360				num = 1;
   361			}
   362			break;
   363		case I2C_SMBUS_BYTE_DATA:
   364			if (read_write == I2C_SMBUS_READ)
   365				msg[1].len = 1;
   366			else {
   367				msg[0].len = 2;
   368				msgbuf0[1] = data->byte;
   369			}
   370			break;
   371		case I2C_SMBUS_WORD_DATA:
   372			if (read_write == I2C_SMBUS_READ)
   373				msg[1].len = 2;
   374			else {
   375				msg[0].len = 3;
   376				msgbuf0[1] = data->word & 0xff;
   377				msgbuf0[2] = data->word >> 8;
   378			}
   379			break;
   380		case I2C_SMBUS_PROC_CALL:
   381			num = 2; /* Special case */
   382			read_write = I2C_SMBUS_READ;
   383			msg[0].len = 3;
   384			msg[1].len = 2;
   385			msgbuf0[1] = data->word & 0xff;
   386			msgbuf0[2] = data->word >> 8;
   387			break;
   388		case I2C_SMBUS_BLOCK_DATA:
   389			if (read_write == I2C_SMBUS_READ) {
   390				msg[1].flags |= I2C_M_RECV_LEN;
   391				msg[1].len = 1; /* block length will be added by
   392						   the underlying bus driver */
   393				i2c_smbus_try_get_dmabuf(&msg[1], 0);
   394			} else {
   395				msg[0].len = data->block[0] + 2;
   396				if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
   397					dev_err(&adapter->dev,
   398						"Invalid block write size %d\n",
   399						data->block[0]);
   400					return -EINVAL;
   401				}
   402	
   403				i2c_smbus_try_get_dmabuf(&msg[0], command);
   404				for (i = 1; i < msg[0].len; i++)
   405					msg[0].buf[i] = data->block[i - 1];
   406			}
   407			break;
   408		case I2C_SMBUS_BLOCK_PROC_CALL:
   409			num = 2; /* Another special case */
   410			read_write = I2C_SMBUS_READ;
   411			if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
   412				dev_err(&adapter->dev,
   413					"Invalid block write size %d\n",
   414					data->block[0]);
   415				return -EINVAL;
   416			}
   417	
   418			msg[0].len = data->block[0] + 2;
   419			i2c_smbus_try_get_dmabuf(&msg[0], command);
   420			for (i = 1; i < msg[0].len; i++)
   421				msg[0].buf[i] = data->block[i - 1];
   422	
   423			msg[1].flags |= I2C_M_RECV_LEN;
   424			msg[1].len = 1; /* block length will be added by
   425					   the underlying bus driver */
   426			i2c_smbus_try_get_dmabuf(&msg[1], 0);
   427			break;
   428		case I2C_SMBUS_I2C_BLOCK_DATA:
   429			if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
   430				dev_err(&adapter->dev, "Invalid block %s size %d\n",
   431					read_write == I2C_SMBUS_READ ? "read" : "write",
   432					data->block[0]);
   433				return -EINVAL;
   434			}
   435	
   436			if (read_write == I2C_SMBUS_READ) {
   437				msg[1].len = data->block[0];
   438				i2c_smbus_try_get_dmabuf(&msg[1], 0);
   439			} else {
   440				msg[0].len = data->block[0] + 1;
   441	
   442				i2c_smbus_try_get_dmabuf(&msg[0], command);
   443				for (i = 1; i <= data->block[0]; i++)
   444					msg[0].buf[i] = data->block[i];
   445			}
   446			break;
   447		default:
   448			dev_err(&adapter->dev, "Unsupported transaction %d\n", size);
   449			return -EOPNOTSUPP;
   450		}
   451	
   452		i = ((flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK
   453					      && size != I2C_SMBUS_I2C_BLOCK_DATA);
   454		if (i) {
   455			/* Compute PEC if first message is a write */
   456			if (!(msg[0].flags & I2C_M_RD)) {
   457				if (num == 1) /* Write only */
   458					i2c_smbus_add_pec(&msg[0]);
   459				else /* Write followed by read */
   460					partial_pec = i2c_smbus_msg_pec(0, &msg[0]);
   461			}
   462			/* Ask for PEC if last message is a read */
   463			if (msg[num-1].flags & I2C_M_RD)
   464				msg[num-1].len++;
   465		}
   466	
   467		status = i2c_transfer(adapter, msg, num);
   468		if (status < 0)
   469			return status;
   470		if (status != num)
   471			return -EIO;
   472	
   473		/* Check PEC if last message is a read */
   474		if (i && (msg[num-1].flags & I2C_M_RD)) {
   475			status = i2c_smbus_check_pec(partial_pec, &msg[num-1]);
   476			if (status < 0)
   477				return status;
   478		}
   479	
   480		if (read_write == I2C_SMBUS_READ)
   481			switch (size) {
   482			case I2C_SMBUS_BYTE:
   483				data->byte = msgbuf0[0];
   484				break;
   485			case I2C_SMBUS_BYTE_DATA:
   486				data->byte = msgbuf1[0];
   487				break;
   488			case I2C_SMBUS_WORD_DATA:
   489			case I2C_SMBUS_PROC_CALL:
   490				data->word = msgbuf1[0] | (msgbuf1[1] << 8);
   491				break;
   492			case I2C_SMBUS_I2C_BLOCK_DATA:
   493				for (i = 0; i < data->block[0]; i++)
   494					data->block[i + 1] = msg[1].buf[i];
   495				break;
   496			case I2C_SMBUS_BLOCK_DATA:
   497			case I2C_SMBUS_BLOCK_PROC_CALL:
   498				for (i = 0; i < msg[1].buf[0] + 1; i++)
   499					data->block[i] = msg[1].buf[i];
   500				break;
   501			}
   502	
   503		if (msg[0].flags & I2C_M_DMA_SAFE)
   504			kfree(msg[0].buf);
   505		if (msg[1].flags & I2C_M_DMA_SAFE)
   506			kfree(msg[1].buf);
   507	
   508		return 0;
   509	}
   510	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Wenwen Wang May 5, 2018, 12:17 p.m. UTC | #4 
On Sat, May 5, 2018 at 5:28 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-05 03:43, Wenwen Wang wrote:
>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>> which are used to save a series of messages, as mentioned in the comment.
>> According to the value of the variable "size", msgbuf0 is initialized to
>> various values. In contrast, msgbuf1 is left uninitialized until the
>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>> initialized on all possible execution paths (implementation) of
>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still be
>> uninitialized even after the invocation of the function i2c_transfer(),
>> especially when the return value of ic2_transfer() is not checked properly.
>> In the following execution, the uninitialized msgbuf1 will be used, such as
>> for security checks. Since uninitialized values can be random and
>> arbitrary, this will cause undefined behaviors or even check bypass. For
>> example, it is expected that if the value of "size" is
>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
>> value read from msgbuf1 is assigned to data->block[0], which can
>> potentially lead to invalid block write size, as demonstrated in the error
>> message.
>>
>> This patch checks the return value of i2c_transfer() and also initializes
>> the first byte of msgbuf1 with 0 to avoid undefined behaviors or security
>> issues.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>>  drivers/i2c/i2c-core-smbus.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>> index b5aec33..e8470d5 100644
>> --- a/drivers/i2c/i2c-core-smbus.c
>> +++ b/drivers/i2c/i2c-core-smbus.c
>> @@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>       };
>>
>>       msgbuf0[0] = command;
>> +     msgbug1[0] = 0;
>>       switch (size) {
>>       case I2C_SMBUS_QUICK:
>>               msg[0].len = 0;
>> @@ -466,6 +467,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>       status = i2c_transfer(adapter, msg, num);
>>       if (status < 0)
>>               return status;
>> +     if (status != num)
>> +             return -EIO;
>>
>>       /* Check PEC if last message is a read */
>>       if (i && (msg[num-1].flags & I2C_M_RD)) {
>>
>
> I think these two hunks should be two separate patches. They address
> orthogonal issues...

Sure, I will split it into two patches and fix the typo :)

Thanks!
Wenwen
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..e8470d5 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -344,6 +344,7 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	};
 
 	msgbuf0[0] = command;
+	msgbug1[0] = 0;
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		msg[0].len = 0;
@@ -466,6 +467,8 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	status = i2c_transfer(adapter, msg, num);
 	if (status < 0)
 		return status;
+	if (status != num)
+		return -EIO;
 
 	/* Check PEC if last message is a read */
 	if (i && (msg[num-1].flags & I2C_M_RD)) {