| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <73945f29-7b1e-4738-ae50-1ae2a9c5c1df@kernel.org>
Date: Tue, 14 Oct 2025 13:03:08 +0200
From: hverkuil+cisco@...nel.org
To: Jeongjun Park <aha310510@...il.com>, mchehab@...nel.org,
hverkuil@...all.nl
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org, syzbot+0192952caa411a3be209@...kaller.appspotmail.com
Subject: Re: [PATCH v2 1/2] media: az6007: fix out-of-bounds in
az6007_i2c_xfer()
On 08/09/2025 17:07, Jeongjun Park wrote:
> Because the blen is not properly bounds-checked in __az6007_read/write,
> it is easy to get out-of-bounds errors in az6007_i2c_xfer later.
>
> Therefore, we need to add bounds-checking to __az6007_read/write to
> resolve this.
>
> Cc: <stable@...r.kernel.org>
> Reported-by: syzbot+0192952caa411a3be209@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0192952caa411a3be209
> Fixes: 786baecfe78f ("[media] dvb-usb: move it to drivers/media/usb/dvb-usb")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> ---
> v2: Change to fix the root cause of oob
> - Link to v1: https://lore.kernel.org/all/20250421105555.34984-1-aha310510@gmail.com/
> ---
> drivers/media/usb/dvb-usb-v2/az6007.c | 62 +++++++++++++++------------
> 1 file changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c
> index 65ef045b74ca..4202042bdb55 100644
> --- a/drivers/media/usb/dvb-usb-v2/az6007.c
> +++ b/drivers/media/usb/dvb-usb-v2/az6007.c
> @@ -97,11 +97,17 @@ static struct mt2063_config az6007_mt2063_config = {
> .refclock = 36125000,
> };
>
> -static int __az6007_read(struct usb_device *udev, u8 req, u16 value,
> - u16 index, u8 *b, int blen)
> +static int __az6007_read(struct usb_device *udev, struct az6007_device_state *st,
> + u8 req, u16 value, u16 index, u8 *b, int blen)
> {
> int ret;
>
> + if (blen > sizeof(st->data)) {
> + pr_err("az6007: tried to read %d bytes, but I2C max size is %lu bytes\n",
> + blen, sizeof(st->data));
> + return -EOPNOTSUPP;
> + }
> +
Hmm, but the pointer 'b' doesn't always point to st->data, so it makes no sense to
check against it.
> ret = usb_control_msg(udev,
> usb_rcvctrlpipe(udev, 0),
> req,
> @@ -125,24 +131,30 @@ static int __az6007_read(struct usb_device *udev, u8 req, u16 value,
> static int az6007_read(struct dvb_usb_device *d, u8 req, u16 value,
> u16 index, u8 *b, int blen)
> {
> - struct az6007_device_state *st = d->priv;
> + struct az6007_device_state *st = d_to_priv(d);
> int ret;
>
> if (mutex_lock_interruptible(&st->mutex) < 0)
> return -EAGAIN;
>
> - ret = __az6007_read(d->udev, req, value, index, b, blen);
> + ret = __az6007_read(d->udev, st, req, value, index, b, blen);
>
> mutex_unlock(&st->mutex);
>
> return ret;
> }
>
> -static int __az6007_write(struct usb_device *udev, u8 req, u16 value,
> - u16 index, u8 *b, int blen)
> +static int __az6007_write(struct usb_device *udev, struct az6007_device_state *st,
> + u8 req, u16 value, u16 index, u8 *b, int blen)
> {
> int ret;
>
> + if (blen > sizeof(st->data)) {
> + pr_err("az6007: tried to write %d bytes, but I2C max size is %lu bytes\n",
> + blen, sizeof(st->data));
> + return -EOPNOTSUPP;
> + }
> +
This makes no sense...
> if (az6007_xfer_debug) {
> printk(KERN_DEBUG "az6007: OUT req: %02x, value: %04x, index: %04x\n",
> req, value, index);
> @@ -150,12 +162,6 @@ static int __az6007_write(struct usb_device *udev, u8 req, u16 value,
> DUMP_PREFIX_NONE, b, blen);
> }
>
> - if (blen > 64) {
> - pr_err("az6007: tried to write %d bytes, but I2C max size is 64 bytes\n",
> - blen);
> - return -EOPNOTSUPP;
> - }
> -
...since it is capped at 64 bytes anyway. So just keep this check since it is more stringent
than sizeof(st->data).
Also, 'b' doesn't always point to st->data, so it makes no sense.
I think this is all overkill.
There are only a few places in this driver where you are reading or writing to/from a
buffer. In most cases the length is hardcoded and clearly fits inside the buffer.
Only is a few places do you need to check that the length <= sizeof(st->data), and
that should just be added as an extra check.
Note that the msg buffers (msg[i].buf) passed to az6007_i2c_xfer are guaranteed to
have the right size for the length (msg[i].len). So you only need to check when
using st->data as the buffer.
Sorry for basically going back to the first patch (almost).
Regards,
Hans
> ret = usb_control_msg(udev,
> usb_sndctrlpipe(udev, 0),
> req,
> @@ -172,13 +178,13 @@ static int __az6007_write(struct usb_device *udev, u8 req, u16 value,
> static int az6007_write(struct dvb_usb_device *d, u8 req, u16 value,
> u16 index, u8 *b, int blen)
> {
> - struct az6007_device_state *st = d->priv;
> + struct az6007_device_state *st = d_to_priv(d);
> int ret;
>
> if (mutex_lock_interruptible(&st->mutex) < 0)
> return -EAGAIN;
>
> - ret = __az6007_write(d->udev, req, value, index, b, blen);
> + ret = __az6007_write(d->udev, st, req, value, index, b, blen);
>
> mutex_unlock(&st->mutex);
>
> @@ -775,7 +781,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> value = addr | (1 << 8);
> length = 6 + msgs[i + 1].len;
> len = msgs[i + 1].len;
> - ret = __az6007_read(d->udev, req, value, index,
> + ret = __az6007_read(d->udev, st, req, value, index,
> st->data, length);
> if (ret >= len) {
> for (j = 0; j < len; j++)
> @@ -788,7 +794,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> if (az6007_xfer_debug)
> printk(KERN_DEBUG "az6007: I2C W addr=0x%x len=%d\n",
> addr, msgs[i].len);
> - if (msgs[i].len < 1) {
> + if (msgs[i].len < 1 && msgs[i].len > 64) {
> ret = -EIO;
> goto err;
> }
> @@ -796,11 +802,8 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> index = msgs[i].buf[0];
> value = addr | (1 << 8);
> length = msgs[i].len - 1;
> - len = msgs[i].len - 1;
> - for (j = 0; j < len; j++)
> - st->data[j] = msgs[i].buf[j + 1];
> - ret = __az6007_write(d->udev, req, value, index,
> - st->data, length);
> + ret = __az6007_write(d->udev, st, req, value, index,
> + &msgs[i].buf[1], length);
> } else {
> /* read bytes */
> if (az6007_xfer_debug)
> @@ -815,10 +818,12 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> value = addr;
> length = msgs[i].len + 6;
> len = msgs[i].len;
> - ret = __az6007_read(d->udev, req, value, index,
> + ret = __az6007_read(d->udev, st, req, value, index,
> st->data, length);
> - for (j = 0; j < len; j++)
> - msgs[i].buf[j] = st->data[j + 5];
> + if (ret >= len) {
> + for (j = 0; j < len; j++)
> + msgs[i].buf[j] = st->data[j + 5];
> + }
> }
> if (ret < 0)
> goto err;
> @@ -845,6 +850,7 @@ static const struct i2c_algorithm az6007_i2c_algo = {
>
> static int az6007_identify_state(struct dvb_usb_device *d, const char **name)
> {
> + struct az6007_device_state *state = d_to_priv(d);
> int ret;
> u8 *mac;
>
> @@ -855,7 +861,7 @@ static int az6007_identify_state(struct dvb_usb_device *d, const char **name)
> return -ENOMEM;
>
> /* Try to read the mac address */
> - ret = __az6007_read(d->udev, AZ6007_READ_DATA, 6, 0, mac, 6);
> + ret = __az6007_read(d->udev, state, AZ6007_READ_DATA, 6, 0, mac, 6);
> if (ret == 6)
> ret = WARM;
> else
> @@ -864,9 +870,9 @@ static int az6007_identify_state(struct dvb_usb_device *d, const char **name)
> kfree(mac);
>
> if (ret == COLD) {
> - __az6007_write(d->udev, 0x09, 1, 0, NULL, 0);
> - __az6007_write(d->udev, 0x00, 0, 0, NULL, 0);
> - __az6007_write(d->udev, 0x00, 0, 0, NULL, 0);
> + __az6007_write(d->udev, state, 0x09, 1, 0, NULL, 0);
> + __az6007_write(d->udev, state, 0x00, 0, 0, NULL, 0);
> + __az6007_write(d->udev, state, 0x00, 0, 0, NULL, 0);
> }
>
> pr_debug("Device is on %s state\n",
> --
>
Powered by blists - more mailing lists