Skip to contentSkip to conversation

staging: greybus: i2c: Use struct_size() helper in gb_i2c_operation_create()

PATCH In review Gustavo A. R. Silva 3 participants 5 messages Jan 21, 2022 – Jan 27, 2022

Subsystems (3)
Staging Greybus Greybus FW/HID/SPI protocols drivers
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 staging: greybus: i2c: Use struct_size() helper in gb_i2c_operation_create()

Make use of the struct_size() helper instead of an open-coded version, in order to avoid any potential type mistakes or integer overflows that, in the worst scenario, could lead to heap overflows.

Also, address the following sparse warnings:
drivers/staging/greybus/i2c.c:111:24: warning: using sizeof on a flexible structure

Link https://github.com/KSPP/linux/issues/174
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/staging/greybus/i2c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/staging/greybus/i2c.c +1 -3
index de2f6516da09..9dfc6791c20e 100644
@@ -108,9 +108,7 @@ gb_i2c_operation_create(struct gb_connection *connection,
 		else
 			data_out_size += (u32)msg->len;
 
-	request_size = sizeof(*request);
-	request_size += msg_count * sizeof(*op);
-	request_size += data_out_size;
+	request_size = struct_size(request, ops, msg_count) + data_out_size;
 
 	/* Response consists only of incoming data */
 	operation = gb_operation_create(connection, GB_I2C_TYPE_TRANSFER,
Signature · 1 line
2.27.0
#1 lore raw
Quoted 18 lines · Gustavo A. R. Silva

Make use of the struct_size() helper instead of an open-coded version, in order to avoid any potential type mistakes or integer overflows that, in the worst scenario, could lead to heap overflows.

Also, address the following sparse warnings:
drivers/staging/greybus/i2c.c:111:24: warning: using sizeof on a flexible structure

Link https://github.com/KSPP/linux/issues/174
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/staging/greybus/i2c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/staging/greybus/i2c.c +0 -0
index de2f6516da09..9dfc6791c20e 100644

drivers/staging/greybus/i2c.c gb_i2c_operation_create(struct gb_connection *connection, · line 111

Quoted 7 lines
drivers/staging/greybus/i2c.c +1 -3
@@ -108,9 +108,7 @@ gb_i2c_operation_create(struct gb_connection *connection,
 		else
 			data_out_size += (u32)msg->len;
 
-	request_size = sizeof(*request);
-	request_size += msg_count * sizeof(*op);
-	request_size += data_out_size;
+	request_size = struct_size(request, ops, msg_count) + data_out_size;

This could still overflow if struct_size() returns SIZE_MAX. Perhaps:

	if (check_add_overflow(struct_size(request, ops, msg_count),
			       data_out_size, &request_size))
		request_size = SIZE_MAX;

I should brush off the saturating arithmetic helpers series:
https://lore.kernel.org/all/[email protected]/

Quoted 6 lines
drivers/staging/greybus/i2c.c +0 -0
 	/* Response consists only of incoming data */
 	operation = gb_operation_create(connection, GB_I2C_TYPE_TRANSFER,
Signature · 1 line
2.27.0
Signature · 1 line
Kees Cook
author
#2 lore raw
Quoted 22 lines · Kees Cook
Quoted 21 lines · Gustavo A. R. Silva

Make use of the struct_size() helper instead of an open-coded version, in order to avoid any potential type mistakes or integer overflows that, in the worst scenario, could lead to heap overflows.

Also, address the following sparse warnings:
drivers/staging/greybus/i2c.c:111:24: warning: using sizeof on a flexible structure

Link https://github.com/KSPP/linux/issues/174
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/staging/greybus/i2c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/staging/greybus/i2c.c +0 -0
index de2f6516da09..9dfc6791c20e 100644
@@ -108,9 +108,7 @@ gb_i2c_operation_create(struct gb_connection *connection,
 		else
 			data_out_size += (u32)msg->len;
Quoted 6 lines

drivers/staging/greybus/i2c.c gb_i2c_operation_create(struct gb_connection *connection, · line 111

Quoted 4 lines
drivers/staging/greybus/i2c.c +1 -3
-	request_size = sizeof(*request);
-	request_size += msg_count * sizeof(*op);
-	request_size += data_out_size;
+	request_size = struct_size(request, ops, msg_count) + data_out_size;

This could still overflow if struct_size() returns SIZE_MAX. Perhaps:

uggh... I got too excited with all the new cases reported.

Quoted 4 lines
	if (check_add_overflow(struct_size(request, ops, msg_count),
			       data_out_size, &request_size))
		request_size = SIZE_MAX;

Yep; I'll respin and include this change, thanks!

Quoted 3 lines

I should brush off the saturating arithmetic helpers series:
https://lore.kernel.org/all/[email protected]/

Yeah; those helpers are very much needed in many places we need to audit...

--
Gustavo

#3 lore raw
In reply to Kees Cook
Quoted 8 lines · Kees Cook

This could still overflow if struct_size() returns SIZE_MAX. Perhaps:

	if (check_add_overflow(struct_size(request, ops, msg_count),
			       data_out_size, &request_size))
		request_size = SIZE_MAX;

I should brush off the saturating arithmetic helpers series:
https://lore.kernel.org/all/[email protected]/

Yes, please! Those seem like a million times easier to use.

regards,
dan carpenter

#4 lore raw
Quoted 11 lines · Dan Carpenter
Quoted 8 lines · Kees Cook

This could still overflow if struct_size() returns SIZE_MAX. Perhaps:

	if (check_add_overflow(struct_size(request, ops, msg_count),
			       data_out_size, &request_size))
		request_size = SIZE_MAX;

I should brush off the saturating arithmetic helpers series:
https://lore.kernel.org/all/[email protected]/

Yes, please! Those seem like a million times easier to use.

Here they are! :) Please review:

https://lore.kernel.org/lkml/[email protected]/

Thanks!

Signature · 1 line
Kees Cook