[PATCH 1/3] i2ctransfer: Don't free memory which was never allocated

[PATCH 1/3] i2ctransfer: Don't free memory which was never allocated

[Jean Delvare]

If an error occurs while msgs[] is been prepared for the transfer,
we jump to the clean-up path. How many buffers need to be freed
depends on the state. If we were parsing data, we should free up to
nmsgs. However, if we were parsing descriptors, we should free
up to nmsgs - 1 only. The code was unconditionally freeing up to
nmsgs, potentially freeing a non-allocated buffer.

In most cases, it was not a problem, we would simply call free() on a
NULL pointer and that's a no-op. However, if msgs[] was full then we
would access memory beyond its end and call free() on a random
pointer.

Fixes: 9fc53a7fc669 ("i2c-tools: add new tool 'i2ctransfer'")
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2ctransfer.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- i2c-tools.orig/tools/i2ctransfer.c
+++ i2c-tools/tools/i2ctransfer.c
@@ -364,7 +364,13 @@ int main(int argc, char *argv[])
  err_out:
 	close(file);
 
-	for (i = 0; i <= nmsgs; i++)
+	/*
+	 * If we were parsing data, the buffer for the last message was
+	 * already allocated and nmsgs still points to it.
+	 */
+	if (state == PARSE_GET_DATA)
+		free(msgs[nmsgs].buf);
+	for (i = 0; i < nmsgs; i++)
 		free(msgs[i].buf);
 
 	exit(1);


-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/3] i2ctransfer: Prevent msgs[] overflow with many parameters

[Jean Delvare]

There's an off-by-one bug in the message count check to ensure that we
do not process more messages than the kernel allows. nmsgs points to
the index within msgs[] which would be used for the _next_ message. If
this index is equal the maximum number of messages then we must stop
already.

This closes bug #220112:
https://bugzilla.kernel.org/show_bug.cgi?id=220112

Fixes: 9fc53a7fc669 ("i2c-tools: add new tool 'i2ctransfer'")
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2ctransfer.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- i2c-tools.orig/tools/i2ctransfer.c
+++ i2c-tools/tools/i2ctransfer.c
@@ -193,7 +193,7 @@ int main(int argc, char *argv[])
 		__u8 data, *buf;
 		char *end;
 
-		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
+		if (nmsgs == I2C_RDRW_IOCTL_MAX_MSGS) {
 			fprintf(stderr, "Error: Too many messages (max: %d)\n",
 				I2C_RDRW_IOCTL_MAX_MSGS);
 			goto err_out;

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3] i2ctransfer: Prevent msgs[] overflow with many parameters

[Wolfram Sang]

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

On Tue, May 13, 2025 at 05:23:30PM +0200, Jean Delvare wrote:
> There's an off-by-one bug in the message count check to ensure that we
> do not process more messages than the kernel allows. nmsgs points to
> the index within msgs[] which would be used for the _next_ message. If
> this index is equal the maximum number of messages then we must stop
> already.
> 
> This closes bug #220112:
> https://bugzilla.kernel.org/show_bug.cgi?id=220112
> 
> Fixes: 9fc53a7fc669 ("i2c-tools: add new tool 'i2ctransfer'")
> Signed-off-by: Jean Delvare <jdelvare@suse.de>

Applied, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] i2ctransfer: Zero out memory passed to ioctl()

[Jean Delvare]

Valgrind complains that uninitialized memory may be passed to
ioctl():

== Syscall param ioctl(I2C_RDWR) points to uninitialised byte(s)
==    at 0x499382B: ioctl (in /lib64/libc.so.6)
==    by 0x401957: main (i2ctransfer.c:343)
==  Address 0x1ffefff94c is on thread 1's stack
==  in frame #1, created by main (i2ctransfer.c:144)
== 
== Syscall param ioctl(I2C_RDWR).msgs points to uninitialised byte(s)
==    at 0x499382B: ioctl (in /lib64/libc.so.6)
==    by 0x401957: main (i2ctransfer.c:343)
==  Address 0x1ffefff956 is on thread 1's stack
==  in frame #1, created by main (i2ctransfer.c:144)

Zero out the i2c_rdwr_ioctl_data struct as well as the msgs array to
guarantee that no uninitialized memory will ever be passed to the
kernel.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
This one is not strictly needed, I can't see any actual bug. However
making valgrind happy seems to be a sane goal, so that we can keep
using it when debugging other issues without getting distracted.
Wolfram, what do you think?

 tools/i2ctransfer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- i2c-tools.orig/tools/i2ctransfer.c
+++ i2c-tools/tools/i2ctransfer.c
@@ -149,8 +149,7 @@ int main(int argc, char *argv[])
 	enum parse_state state = PARSE_GET_DESC;
 	unsigned int buf_idx = 0;
 
-	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
-		msgs[i].buf = NULL;
+	memset(msgs, 0, sizeof(msgs));
 
 	/* handle (optional) flags first */
 	while ((opt = getopt(argc, argv, "abfhvVy")) != -1) {
@@ -334,6 +333,7 @@ int main(int argc, char *argv[])
 		struct i2c_rdwr_ioctl_data rdwr;
 		unsigned int print_flags = PRINT_READ_BUF;
 
+		memset(&rdwr, 0, sizeof(rdwr));
 		rdwr.msgs = msgs;
 		rdwr.nmsgs = nmsgs;
 		nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/3] i2ctransfer: Zero out memory passed to ioctl()

[Wolfram Sang]

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

On Tue, May 13, 2025 at 05:35:39PM +0200, Jean Delvare wrote:
> Valgrind complains that uninitialized memory may be passed to
> ioctl():
> 
> == Syscall param ioctl(I2C_RDWR) points to uninitialised byte(s)
> ==    at 0x499382B: ioctl (in /lib64/libc.so.6)
> ==    by 0x401957: main (i2ctransfer.c:343)
> ==  Address 0x1ffefff94c is on thread 1's stack
> ==  in frame #1, created by main (i2ctransfer.c:144)
> == 
> == Syscall param ioctl(I2C_RDWR).msgs points to uninitialised byte(s)
> ==    at 0x499382B: ioctl (in /lib64/libc.so.6)
> ==    by 0x401957: main (i2ctransfer.c:343)
> ==  Address 0x1ffefff956 is on thread 1's stack
> ==  in frame #1, created by main (i2ctransfer.c:144)
> 
> Zero out the i2c_rdwr_ioctl_data struct as well as the msgs array to
> guarantee that no uninitialized memory will ever be passed to the
> kernel.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>

Applied, thanks!

> This one is not strictly needed, I can't see any actual bug. However
> making valgrind happy seems to be a sane goal, so that we can keep
> using it when debugging other issues without getting distracted.
> Wolfram, what do you think?

Sure, I agree. Also, I like the simpler code.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] i2ctransfer: Don't free memory which was never allocated

[Wolfram Sang]

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

On Tue, May 13, 2025 at 05:21:19PM +0200, Jean Delvare wrote:
> If an error occurs while msgs[] is been prepared for the transfer,
> we jump to the clean-up path. How many buffers need to be freed
> depends on the state. If we were parsing data, we should free up to
> nmsgs. However, if we were parsing descriptors, we should free
> up to nmsgs - 1 only. The code was unconditionally freeing up to
> nmsgs, potentially freeing a non-allocated buffer.
> 
> In most cases, it was not a problem, we would simply call free() on a
> NULL pointer and that's a no-op. However, if msgs[] was full then we
> would access memory beyond its end and call free() on a random
> pointer.
> 
> Fixes: 9fc53a7fc669 ("i2c-tools: add new tool 'i2ctransfer'")
> Signed-off-by: Jean Delvare <jdelvare@suse.de>

Applied, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]