i2c: cp2615: prevent buffer overflow in cp2615_i2c_master_xfer()

PATCH In review Dan Carpenter 2 participants 5 messages Oct 12, 2022 – Oct 14, 2022

Subsystems (3)

Kernel janitors I2C subsystem host drivers CP2615 I2C driver

lore

Share & tools

lore

Dan Carpenter

author

2022-10-12 14:52 UTC #0 lore raw

PATCH 1/1 i2c: cp2615: prevent buffer overflow in cp2615_i2c_master_xfer()

The "msg->len" can be controlled by the user via the ioctl. We need to ensure that it is not too large.

Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/i2c/busses/i2c-cp2615.c +2 -0

index 3ded28632e4c..ad1d6e548503 100644
@@ -231,6 +231,8 @@ cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		} else {
 			i2c_w.read_len = 0;
 			i2c_w.write_len = msg->len;
+			if (msg->len > sizeof(i2c_w.data))
+				return -EINVAL;
 			memcpy(&i2c_w.data, msg->buf, i2c_w.write_len);
 		}
 		ret = cp2615_i2c_send(usbif, &i2c_w);

Signature · 1 line

2.35.1

Bence Csókás

maintainer

2022-10-13 18:55 UTC #1 lore raw

Dan Carpenter <[email protected]> ezt írta (időpont: 2022. okt. 12., Sze, 16:52):

Quoted 3 lines

The "msg->len" can be controlled by the user via the ioctl. We need to ensure that it is not too large.

Does the I2C core not check that submitted msgs do not exceed maximums specified in i2c_adapter_quirks? @WSA?
If not, other drivers may also have this issue.

Quoted 11 lines

Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/i2c/busses/i2c-cp2615.c +0 -0
index 3ded28632e4c..ad1d6e548503 100644

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) · line 235

Quoted 5 lines

drivers/i2c/busses/i2c-cp2615.c +2 -0

@@ -231,6 +231,8 @@ cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
                } else {
                        i2c_w.read_len = 0;
                        i2c_w.write_len = msg->len;
+                       if (msg->len > sizeof(i2c_w.data))
+                               return -EINVAL;

Please move this up to line 225, as an invalid read_len is also an error and should bail out accordingly.

Quoted 7 lines

drivers/i2c/busses/i2c-cp2615.c +0 -1

                        memcpy(&i2c_w.data, msg->buf, i2c_w.write_len);
                }
                ret = cp2615_i2c_send(usbif, &i2c_w);
--

2.35.1

Bence

Dan Carpenter

author

2022-10-14 07:03 UTC #2 lore raw

Quoted 10 lines · Bence Csókás

Dan Carpenter <[email protected]> ezt írta (időpont: 2022. okt. 12., Sze, 16:52):
Quoted 3 lines
The "msg->len" can be controlled by the user via the ioctl. We need to ensure that it is not too large.
Does the I2C core not check that submitted msgs do not exceed maximums specified in i2c_adapter_quirks? @WSA?
If not, other drivers may also have this issue.

I fixed another driver as well.

The other related bugs we've been fixing recently are related to i2cdev_ioctl(). So it's not 100% the same, but similar.

Quoted 6 lines

Quoted 6 lines
Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 14 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) · line 235

Quoted 10 lines

drivers/i2c/busses/i2c-cp2615.c +2 -0

index 3ded28632e4c..ad1d6e548503 100644
@@ -231,6 +231,8 @@ cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
                } else {
                        i2c_w.read_len = 0;
                        i2c_w.write_len = msg->len;
+                       if (msg->len > sizeof(i2c_w.data))
+                               return -EINVAL;

Please move this up to line 225, as an invalid read_len is also an error and should bail out accordingly.

I don't see the bug. Is that something that requires knowledge of the hardware?

regards,
dan carpenter

Bence Csókás

maintainer

2022-10-14 11:17 UTC #3 lore raw

Dan Carpenter <[email protected]> ezt írta (időpont: 2022. okt. 14., P, 9:03):

Quoted 18 lines

Quoted 17 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) · line 235

Quoted 13 lines

 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/i2c/busses/i2c-cp2615.c +2 -0

index 3ded28632e4c..ad1d6e548503 100644
@@ -231,6 +231,8 @@ cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
                } else {
                        i2c_w.read_len = 0;
                        i2c_w.write_len = msg->len;
+                       if (msg->len > sizeof(i2c_w.data))
+                               return -EINVAL;

Please move this up to line 225, as an invalid read_len is also an error and should bail out accordingly.

Quoted 2 lines

I don't see the bug. Is that something that requires knowledge of the hardware?

No, what I mean is that you put the check in the else clause of

Quoted 1 line

if (msg->flags & I2C_M_RD) {

But a msg->len > MAX_I2C_SIZE is invalid, regardless of msg->flags. So the check should be outside if the if.

Quoted 2 lines

regards,
dan carpenter

Bence

Dan Carpenter

author

2022-10-14 14:29 UTC #4 lore raw

Quoted 20 lines · Bence Csókás

Dan Carpenter <[email protected]> ezt írta (időpont: 2022. okt. 14., P, 9:03):
Quoted 18 lines
Quoted 17 lines
drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) · line 235
Quoted 13 lines
 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/i2c/busses/i2c-cp2615.c +2 -0
index 3ded28632e4c..ad1d6e548503 100644
@@ -231,6 +231,8 @@ cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
                } else {
                        i2c_w.read_len = 0;
                        i2c_w.write_len = msg->len;
+                       if (msg->len > sizeof(i2c_w.data))
+                               return -EINVAL;
Please move this up to line 225, as an invalid read_len is also an error and should bail out accordingly.

Quoted 8 lines

Quoted 2 lines
I don't see the bug. Is that something that requires knowledge of the hardware?
No, what I mean is that you put the check in the else clause of
Quoted 1 line
if (msg->flags & I2C_M_RD) {
But a msg->len > MAX_I2C_SIZE is invalid, regardless of msg->flags. So the check should be outside if the if.

Hm... I was looking at how that could be added at a lower level and actually the quirks code you mentioned earlier takes care of this in i2c_check_for_quirks().

So this patch is not required. Please drop it. Sorry for the noise.

regards,
dan carpenter