| Message ID | 20260818124737.64478-1-triet.hoang.dev@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: cp2615: Validate read length before copying | expand |
… > Limit read_len to the maximum size of the response buffer and return > -EPROTO for an invalid response. * How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n145 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2#n34 * Please avoid duplicate source code also for improved implementations of functions like cp2615_i2c_recv(). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.2#n526 https://elixir.bootlin.com/linux/v7.2/source/drivers/i2c/busses/i2c-cp2615.c#L138-L169 * How do you think about to increase the application of scope-based resource management? Regards, Markus
Hi Triet, On Tue, Aug 18, 2026 at 12:47:37PM +0000, Triet Hoang wrote: > The read_len field comes from the untrusted USB payload, which could > potentially exceed the client's originally requested buffer length or > MAX_I2C_SIZE, allowing an out-of-bounds read and write. have you experienced any issue yourself or are you just speculating based on the code? Andi > Limit read_len to the maximum size of the response buffer and return > -EPROTO for an invalid response. > > Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
On Thu, 20 Aug 2026 10:28:22 -0500, Andi wrote > have you experienced any issue yourself or are you just > speculating based on the code? Hi Andi, Thanks for replying. I just speculating based on the code. BR, Triet
Hi Triet, On Thu, Aug 20, 2026 at 03:40:41PM +0000, Triet Hoang wrote: > On Thu, 20 Aug 2026 10:28:22 -0500, Andi wrote > > have you experienced any issue yourself or are you just > > speculating based on the code? > > Thanks for replying. I just speculating based on the code. I'm sorry, but I'm going to drop this as I don't see it as necessary. As Markus pointed out, if this failure could actually happen, it would result in a driver failure and the patch should then be treated as a bug fix. However, I don't want to fix hypothetical bugs based on speculation or an AI-generated guess. Thanks, Andi
diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c index 951de6249834..2c79530da306 100644 --- a/drivers/i2c/busses/i2c-cp2615.c +++ b/drivers/i2c/busses/i2c-cp2615.c @@ -136,7 +136,7 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) } static int -cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf) +cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf, int len) { struct usb_device *usbdev = interface_to_usbdev(usbif); struct cp2615_iop_msg *msg; @@ -160,6 +160,11 @@ cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf) return -EIO; } + if (i2c_r->read_len > len || i2c_r->read_len > MAX_I2C_SIZE) { + kfree(msg); + return -EPROTO; + } + res = cp2615_check_status(i2c_r->status); if (!res) memcpy(buf, &i2c_r->data, i2c_r->read_len); @@ -236,7 +241,7 @@ cp2615_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) ret = cp2615_i2c_send(usbif, &i2c_w); if (ret) break; - ret = cp2615_i2c_recv(usbif, i2c_w.tag, msg->buf); + ret = cp2615_i2c_recv(usbif, i2c_w.tag, msg->buf, msg->len); } if (ret < 0) return ret;
The read_len field comes from the untrusted USB payload, which could potentially exceed the client's originally requested buffer length or MAX_I2C_SIZE, allowing an out-of-bounds read and write. Limit read_len to the maximum size of the response buffer and return -EPROTO for an invalid response. Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com> --- drivers/i2c/busses/i2c-cp2615.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)