[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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)) {
-- 
2.7.4

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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)) {
-- 
2.7.4

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7742 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31891 bytes --]

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7742 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31891 bytes --]

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

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

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7777 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23667 bytes --]

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7777 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23667 bytes --]

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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 not be
uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
behaviors or security issues.

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

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..0fcca75 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	 * somewhat simpler.
 	 */
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
 	int i;
 	u8 partial_pec = 0;
-- 
2.7.4

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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 not be
uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
behaviors or security issues.

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

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..0fcca75 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	 * somewhat simpler.
 	 */
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
 	int i;
 	u8 partial_pec = 0;
-- 
2.7.4

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-03 00:36, 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 not be

double negation here

> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
> behaviors or security issues.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/i2c/i2c-core-smbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..0fcca75 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  	 * somewhat simpler.
>  	 */
>  	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
> -	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
> +	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};

I think this will result in the whole of msgbuf1 being filled with zeroes.
It might be cheaper to do this with code proper rather than with an
initializer?

Cheers,
Peter

>  	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>  	int i;
>  	u8 partial_pec = 0;
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-03 00:36, 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 not be
>
> double negation here
>
>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>> behaviors or security issues.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>> index b5aec33..0fcca75 100644
>> --- a/drivers/i2c/i2c-core-smbus.c
>> +++ b/drivers/i2c/i2c-core-smbus.c
>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>        * somewhat simpler.
>>        */
>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>
> I think this will result in the whole of msgbuf1 being filled with zeroes.
> It might be cheaper to do this with code proper rather than with an
> initializer?

Thanks for your comment, Peter!  How about using a memset() only when
i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
used only in that case?

Thanks,
Wenwen

>
> Cheers,
> Peter
>
>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>       int i;
>>       u8 partial_pec = 0;
>>
>

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 06:08, Wenwen Wang wrote:
> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-03 00:36, 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 not be
>>
>> double negation here
>>
>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>> behaviors or security issues.
>>>
>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>> ---
>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index b5aec33..0fcca75 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>        * somewhat simpler.
>>>        */
>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>
>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>> It might be cheaper to do this with code proper rather than with an
>> initializer?
> 
> Thanks for your comment, Peter!  How about using a memset() only when
> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
> used only in that case?

I was thinking that an assignment of

	msgbuf1[0] = 0;

would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
cases before the i2c_transfer call. However, this will only kick in if
the call to kzalloc fails (and it most likely will not) in the call to the
i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
seems like a non-issue to me.

However, while looking I think the bigger problem with that function is that
it considers all non-negative return values from i2c_transfer as good<tm>.
IMHO, it should barf on any return values <> num. Or at the very least
describe why a partial result is considered OK...

Cheers,
Peter

>>
>> Cheers,
>> Peter
>>
>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>       int i;
>>>       u8 partial_pec = 0;
>>>
>>

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 06:08, Wenwen Wang wrote:
>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-03 00:36, 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 not be
>>>
>>> double negation here
>>>
>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>> behaviors or security issues.
>>>>
>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>> ---
>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>> index b5aec33..0fcca75 100644
>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>        * somewhat simpler.
>>>>        */
>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>
>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>> It might be cheaper to do this with code proper rather than with an
>>> initializer?
>>
>> Thanks for your comment, Peter!  How about using a memset() only when
>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>> used only in that case?
>
> I was thinking that an assignment of
>
>         msgbuf1[0] = 0;
>
> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
> cases before the i2c_transfer call. However, this will only kick in if
> the call to kzalloc fails (and it most likely will not) in the call to the
> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
> seems like a non-issue to me.
>
> However, while looking I think the bigger problem with that function is that
> it considers all non-negative return values from i2c_transfer as good<tm>.
> IMHO, it should barf on any return values <> num. Or at the very least
> describe why a partial result is considered OK...
>
> Cheers,
> Peter
>
>>>
>>> Cheers,
>>> Peter
>>>
>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>       int i;
>>>>       u8 partial_pec = 0;
>>>>
>>>
>

Yes, it is a big issue if the return value from i2c_transfer() is not
equal to num. I can add a check like this:

if (status != num)
  return -EINVAL;

Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
in i2c_smbus_try_get_dmabuf()?

Thanks,
Wenwen

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 07:28, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-03 00:36, 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 not be
>>>>
>>>> double negation here
>>>>
>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>> behaviors or security issues.
>>>>>
>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>> ---
>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>> index b5aec33..0fcca75 100644
>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>        * somewhat simpler.
>>>>>        */
>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>
>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>> It might be cheaper to do this with code proper rather than with an
>>>> initializer?
>>>
>>> Thanks for your comment, Peter!  How about using a memset() only when
>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>> used only in that case?
>>
>> I was thinking that an assignment of
>>
>>         msgbuf1[0] = 0;
>>
>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>> cases before the i2c_transfer call. However, this will only kick in if
>> the call to kzalloc fails (and it most likely will not) in the call to the
>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>> seems like a non-issue to me.
>>
>> However, while looking I think the bigger problem with that function is that
>> it considers all non-negative return values from i2c_transfer as good<tm>.
>> IMHO, it should barf on any return values <> num. Or at the very least
>> describe why a partial result is considered OK...
>>
>> Cheers,
>> Peter
>>
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>       int i;
>>>>>       u8 partial_pec = 0;
>>>>>
>>>>
>>
> 
> Yes, it is a big issue if the return value from i2c_transfer() is not
> equal to num. I can add a check like this:
> 
> if (status != num)
>   return -EINVAL;
> 

Right, but make sure to add it *after* the existing "if (status < 0)"
check as we want to preserve any existing error. Also, -EIO is perhaps
more appropriate than -EINVAL which seems wrong for what is probably
a runtime incident.

> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
> in i2c_smbus_try_get_dmabuf()?

It is not always replaced. The stack buffer is probably retained as
the default mode of operation (and fallback) because kzalloc is
expensive and because kzalloc might fail?

Cheers,
Peter

> Thanks,
> Wenwen
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 07:28, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-03 00:36, 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 not be
>>>>>
>>>>> double negation here
>>>>>
>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>> behaviors or security issues.
>>>>>>
>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>> ---
>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>> index b5aec33..0fcca75 100644
>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>        * somewhat simpler.
>>>>>>        */
>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>
>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>> It might be cheaper to do this with code proper rather than with an
>>>>> initializer?
>>>>
>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>> used only in that case?
>>>
>>> I was thinking that an assignment of
>>>
>>>         msgbuf1[0] = 0;
>>>
>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>> cases before the i2c_transfer call. However, this will only kick in if
>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>> seems like a non-issue to me.
>>>
>>> However, while looking I think the bigger problem with that function is that
>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>> IMHO, it should barf on any return values <> num. Or at the very least
>>> describe why a partial result is considered OK...
>>>
>>> Cheers,
>>> Peter
>>>
>>>>>
>>>>> Cheers,
>>>>> Peter
>>>>>
>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>       int i;
>>>>>>       u8 partial_pec = 0;
>>>>>>
>>>>>
>>>
>>
>> Yes, it is a big issue if the return value from i2c_transfer() is not
>> equal to num. I can add a check like this:
>>
>> if (status != num)
>>   return -EINVAL;
>>
>
> Right, but make sure to add it *after* the existing "if (status < 0)"
> check as we want to preserve any existing error. Also, -EIO is perhaps
> more appropriate than -EINVAL which seems wrong for what is probably
> a runtime incident.
>

Sure, I will place it after the existing check and replace -EINVAL with -EIO.

>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>> in i2c_smbus_try_get_dmabuf()?
>
> It is not always replaced. The stack buffer is probably retained as
> the default mode of operation (and fallback) because kzalloc is
> expensive and because kzalloc might fail?
>

That means the stack buffer is probably used if kzalloc is failed.
Actually, the kzalloc failure would be possible if a user-space
process maliciously causes the kernel to consume a large chunk of
memory. In that case, the user can potentially exploit this
problematic code. So it may be better to initialize the stack buffer.

Thanks,
Wenwen

> Cheers,
> Peter
>
>> Thanks,
>> Wenwen
>>
>

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 09:17, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>> On 2018-05-03 00:36, 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 not be
>>>>>>
>>>>>> double negation here
>>>>>>
>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>> behaviors or security issues.
>>>>>>>
>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>> ---
>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>> index b5aec33..0fcca75 100644
>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>        * somewhat simpler.
>>>>>>>        */
>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>
>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>> initializer?
>>>>>
>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>> used only in that case?
>>>>
>>>> I was thinking that an assignment of
>>>>
>>>>         msgbuf1[0] = 0;
>>>>
>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>> seems like a non-issue to me.
>>>>
>>>> However, while looking I think the bigger problem with that function is that
>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>> describe why a partial result is considered OK...
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Peter
>>>>>>
>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>       int i;
>>>>>>>       u8 partial_pec = 0;
>>>>>>>
>>>>>>
>>>>
>>>
>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>> equal to num. I can add a check like this:
>>>
>>> if (status != num)
>>>   return -EINVAL;
>>>
>>
>> Right, but make sure to add it *after* the existing "if (status < 0)"
>> check as we want to preserve any existing error. Also, -EIO is perhaps
>> more appropriate than -EINVAL which seems wrong for what is probably
>> a runtime incident.
>>
> 
> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
> 
>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>> in i2c_smbus_try_get_dmabuf()?
>>
>> It is not always replaced. The stack buffer is probably retained as
>> the default mode of operation (and fallback) because kzalloc is
>> expensive and because kzalloc might fail?
>>
> 
> That means the stack buffer is probably used if kzalloc is failed.
> Actually, the kzalloc failure would be possible if a user-space
> process maliciously causes the kernel to consume a large chunk of
> memory. In that case, the user can potentially exploit this
> problematic code. So it may be better to initialize the stack buffer.

Yes, but I see little reason to initialize more than the first byte.

You hinted in the commit message that there were execution paths (or
implementations) where the second buffer wasn't initialized. Can you
give an example where this matters when the more extensive check on
the i2c_transfer return value is in place? That seems like a bugs
that should *also* be fixed in the affected i2c bus drivers...

Cheers,
Peter

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 09:17, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>> On 2018-05-03 00:36, 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 not be
>>>>>>>
>>>>>>> double negation here
>>>>>>>
>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>> behaviors or security issues.
>>>>>>>>
>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>> ---
>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>        * somewhat simpler.
>>>>>>>>        */
>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>
>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>> initializer?
>>>>>>
>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>> used only in that case?
>>>>>
>>>>> I was thinking that an assignment of
>>>>>
>>>>>         msgbuf1[0] = 0;
>>>>>
>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>> seems like a non-issue to me.
>>>>>
>>>>> However, while looking I think the bigger problem with that function is that
>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>> describe why a partial result is considered OK...
>>>>>
>>>>> Cheers,
>>>>> Peter
>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Peter
>>>>>>>
>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>       int i;
>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>> equal to num. I can add a check like this:
>>>>
>>>> if (status != num)
>>>>   return -EINVAL;
>>>>
>>>
>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>> more appropriate than -EINVAL which seems wrong for what is probably
>>> a runtime incident.
>>>
>>
>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>
>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>> in i2c_smbus_try_get_dmabuf()?
>>>
>>> It is not always replaced. The stack buffer is probably retained as
>>> the default mode of operation (and fallback) because kzalloc is
>>> expensive and because kzalloc might fail?
>>>
>>
>> That means the stack buffer is probably used if kzalloc is failed.
>> Actually, the kzalloc failure would be possible if a user-space
>> process maliciously causes the kernel to consume a large chunk of
>> memory. In that case, the user can potentially exploit this
>> problematic code. So it may be better to initialize the stack buffer.
>
> Yes, but I see little reason to initialize more than the first byte.
>
> You hinted in the commit message that there were execution paths (or
> implementations) where the second buffer wasn't initialized. Can you
> give an example where this matters when the more extensive check on
> the i2c_transfer return value is in place? That seems like a bugs
> that should *also* be fixed in the affected i2c bus drivers...

One possible execution path is as follows:

i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
master_xfer handlers)

In pca_xfer(), it reads the status of the i2c_adapter and then
performs different actions according to different statuses.

It seems probable that the buffer is not filled with the wanted data
if the status is not as expected.

Wenwen

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 16:59, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>>> On 2018-05-03 00:36, 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 not be
>>>>>>>>
>>>>>>>> double negation here
>>>>>>>>
>>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>>> behaviors or security issues.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>>> ---
>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>>        * somewhat simpler.
>>>>>>>>>        */
>>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>
>>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>> initializer?
>>>>>>>
>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>>> used only in that case?
>>>>>>
>>>>>> I was thinking that an assignment of
>>>>>>
>>>>>>         msgbuf1[0] = 0;
>>>>>>
>>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>>> seems like a non-issue to me.
>>>>>>
>>>>>> However, while looking I think the bigger problem with that function is that
>>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>>> describe why a partial result is considered OK...
>>>>>>
>>>>>> Cheers,
>>>>>> Peter
>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Peter
>>>>>>>>
>>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>>       int i;
>>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>>> equal to num. I can add a check like this:
>>>>>
>>>>> if (status != num)
>>>>>   return -EINVAL;
>>>>>
>>>>
>>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>>> more appropriate than -EINVAL which seems wrong for what is probably
>>>> a runtime incident.
>>>>
>>>
>>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>>
>>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>>> in i2c_smbus_try_get_dmabuf()?
>>>>
>>>> It is not always replaced. The stack buffer is probably retained as
>>>> the default mode of operation (and fallback) because kzalloc is
>>>> expensive and because kzalloc might fail?
>>>>
>>>
>>> That means the stack buffer is probably used if kzalloc is failed.
>>> Actually, the kzalloc failure would be possible if a user-space
>>> process maliciously causes the kernel to consume a large chunk of
>>> memory. In that case, the user can potentially exploit this
>>> problematic code. So it may be better to initialize the stack buffer.
>>
>> Yes, but I see little reason to initialize more than the first byte.
>>
>> You hinted in the commit message that there were execution paths (or
>> implementations) where the second buffer wasn't initialized. Can you
>> give an example where this matters when the more extensive check on
>> the i2c_transfer return value is in place? That seems like a bugs
>> that should *also* be fixed in the affected i2c bus drivers...
> 
> One possible execution path is as follows:
> 
> i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
> master_xfer handlers)
> 
> In pca_xfer(), it reads the status of the i2c_adapter and then
> performs different actions according to different statuses.
> 
> It seems probable that the buffer is not filled with the wanted data
> if the status is not as expected.

Ah, so you're talking about hardware malfunction without any actual
real-life incident. In other words, pure speculation. I'm sure the
kernel is full of problems if every potential HW misbehavior is
considered, and I'm not so sure this particular problem is going
to matter all that much...

Cheers,
Peter

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

On Fri, May 4, 2018 at 10:38 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-04 16:59, Wenwen Wang wrote:
>> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <peda@axentia.se> wrote:
>>>>>>>>> On 2018-05-03 00:36, 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 not be
>>>>>>>>>
>>>>>>>>> double negation here
>>>>>>>>>
>>>>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>>>>>>> behaviors or security issues.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>>>>>>>>        * somewhat simpler.
>>>>>>>>>>        */
>>>>>>>>>>       unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>>> -     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>>> +     unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>>
>>>>>>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>>> initializer?
>>>>>>>>
>>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>>>> used only in that case?
>>>>>>>
>>>>>>> I was thinking that an assignment of
>>>>>>>
>>>>>>>         msgbuf1[0] = 0;
>>>>>>>
>>>>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>>>>> seems like a non-issue to me.
>>>>>>>
>>>>>>> However, while looking I think the bigger problem with that function is that
>>>>>>> it considers all non-negative return values from i2c_transfer as good<tm>.
>>>>>>> IMHO, it should barf on any return values <> num. Or at the very least
>>>>>>> describe why a partial result is considered OK...
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Peter
>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Peter
>>>>>>>>>
>>>>>>>>>>       int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>>>>>>       int i;
>>>>>>>>>>       u8 partial_pec = 0;
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Yes, it is a big issue if the return value from i2c_transfer() is not
>>>>>> equal to num. I can add a check like this:
>>>>>>
>>>>>> if (status != num)
>>>>>>   return -EINVAL;
>>>>>>
>>>>>
>>>>> Right, but make sure to add it *after* the existing "if (status < 0)"
>>>>> check as we want to preserve any existing error. Also, -EIO is perhaps
>>>>> more appropriate than -EINVAL which seems wrong for what is probably
>>>>> a runtime incident.
>>>>>
>>>>
>>>> Sure, I will place it after the existing check and replace -EINVAL with -EIO.
>>>>
>>>>>> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
>>>>>> in i2c_smbus_try_get_dmabuf()?
>>>>>
>>>>> It is not always replaced. The stack buffer is probably retained as
>>>>> the default mode of operation (and fallback) because kzalloc is
>>>>> expensive and because kzalloc might fail?
>>>>>
>>>>
>>>> That means the stack buffer is probably used if kzalloc is failed.
>>>> Actually, the kzalloc failure would be possible if a user-space
>>>> process maliciously causes the kernel to consume a large chunk of
>>>> memory. In that case, the user can potentially exploit this
>>>> problematic code. So it may be better to initialize the stack buffer.
>>>
>>> Yes, but I see little reason to initialize more than the first byte.
>>>
>>> You hinted in the commit message that there were execution paths (or
>>> implementations) where the second buffer wasn't initialized. Can you
>>> give an example where this matters when the more extensive check on
>>> the i2c_transfer return value is in place? That seems like a bugs
>>> that should *also* be fixed in the affected i2c bus drivers...
>>
>> One possible execution path is as follows:
>>
>> i2c_transfer -> __i2c_transfer -> pca_xfer (which is one of the
>> master_xfer handlers)
>>
>> In pca_xfer(), it reads the status of the i2c_adapter and then
>> performs different actions according to different statuses.
>>
>> It seems probable that the buffer is not filled with the wanted data
>> if the status is not as expected.
>
> Ah, so you're talking about hardware malfunction without any actual
> real-life incident. In other words, pure speculation. I'm sure the
> kernel is full of problems if every potential HW misbehavior is
> considered, and I'm not so sure this particular problem is going
> to matter all that much...

Thanks for your comments, Peter! I will submit a new patch :)

Wenwen

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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 not be
uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
behaviors or security issues.

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

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..0fcca75 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	 * somewhat simpler.
 	 */
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
 	int i;
 	u8 partial_pec = 0;
-- 
2.7.4

[PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Wenwen Wang]

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 not be
uninitialized even after the invocation of the function i2c_transfer(). 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 simply initializes the buffer msgbuf1 with 0 to avoid undefined
behaviors or security issues.

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

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b5aec33..0fcca75 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	 * somewhat simpler.
 	 */
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
 	int i;
 	u8 partial_pec = 0;
-- 
2.7.4