diff mbox series

i2c: cp2615: prevent buffer overflow in cp2615_i2c_master_xfer()

Message ID Y0bUdt73moVmaajb@kili
State New
Headers show
Series i2c: cp2615: prevent buffer overflow in cp2615_i2c_master_xfer() | expand

Commit Message

Dan Carpenter Oct. 12, 2022, 2:51 p.m. UTC 
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 <dan.carpenter@oracle.com>
---
 drivers/i2c/busses/i2c-cp2615.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Bence Csókás Oct. 13, 2022, 6:02 p.m. UTC | #1 
Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2022. okt.
12., Sze, 16:52):
>
> 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.

> Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/i2c/busses/i2c-cp2615.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> index 3ded28632e4c..ad1d6e548503 100644
> --- a/drivers/i2c/busses/i2c-cp2615.c
> +++ b/drivers/i2c/busses/i2c-cp2615.c
> @@ -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.

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

Bence
Dan Carpenter Oct. 14, 2022, 7:03 a.m. UTC | #2 
On Thu, Oct 13, 2022 at 08:02:04PM +0200, Bence Csókás wrote:
> Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2022. okt.
> 12., Sze, 16:52):
> >
> > 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.


> > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/i2c/busses/i2c-cp2615.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> > index 3ded28632e4c..ad1d6e548503 100644
> > --- a/drivers/i2c/busses/i2c-cp2615.c
> > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > @@ -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 Oct. 14, 2022, 11:17 a.m. UTC | #3 
Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2022. okt.
14., P, 9:03):
> > >  drivers/i2c/busses/i2c-cp2615.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> > > index 3ded28632e4c..ad1d6e548503 100644
> > > --- a/drivers/i2c/busses/i2c-cp2615.c
> > > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > > @@ -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?

No, what I mean is that you put the check in the else clause of
> 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`.

> regards,
> dan carpenter

Bence
Dan Carpenter Oct. 14, 2022, 2:23 p.m. UTC | #4 
On Fri, Oct 14, 2022 at 01:17:32PM +0200, Bence Csókás wrote:
> Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2022. okt.
> 14., P, 9:03):
> > > >  drivers/i2c/busses/i2c-cp2615.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> > > > index 3ded28632e4c..ad1d6e548503 100644
> > > > --- a/drivers/i2c/busses/i2c-cp2615.c
> > > > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > > > @@ -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?
> 
> No, what I mean is that you put the check in the else clause of
> > 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
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
index 3ded28632e4c..ad1d6e548503 100644
--- a/drivers/i2c/busses/i2c-cp2615.c
+++ b/drivers/i2c/busses/i2c-cp2615.c
@@ -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);