| 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: <1b41205e-9af2-487f-889c-d6fb328dfbb2@kernel.org>
Date: Tue, 14 Oct 2025 12:29:37 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jeongjun Park <aha310510@...il.com>, mchehab@...nel.org,
hverkuil@...nel.org
Cc: christophe.jaillet@...adoo.fr, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: dvb-usb: az6027: fix out-of-bounds in
az6027_i2c_xfer()
On 07/09/2025 15:53, Jeongjun Park wrote:
> msg[i].len is a user-controlled value, the current implementation easily
> causes out-of-bounds errors in az6027_i2c_xfer().
>
> Therefore, to prevent this, we need to strengthen bounds checking through
> a comprehensive refactoring of az6027_usb_in_op/az6027_usb_out_op/
> az6027_i2c_xfer.
>
> 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/20250421115045.81394-1-aha310510@gmail.com/
> ---
> drivers/media/usb/dvb-usb/az6027.c | 105 ++++++++++++++++++++---------
> 1 file changed, 72 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb/az6027.c b/drivers/media/usb/dvb-usb/az6027.c
> index 056935d3cbd6..c021362e9b8a 100644
> --- a/drivers/media/usb/dvb-usb/az6027.c
> +++ b/drivers/media/usb/dvb-usb/az6027.c
> @@ -296,12 +296,19 @@ static struct stb6100_config az6027_stb6100_config = {
>
> /* check for mutex FIXME */
> static int az6027_usb_in_op(struct dvb_usb_device *d, u8 req,
> - u16 value, u16 index, u8 *b, int blen)
> + u16 value, u16 index, u8 *b, int blen, int buf_size)
> {
> int ret = -1;
> if (mutex_lock_interruptible(&d->usb_mutex))
> return -EAGAIN;
>
> + if (blen > buf_size) {
> + err("usb in %d bytes, but max size is %d bytes\n",
> + blen, buf_size);
> + ret = -EOPNOTSUPP;
> + goto end_unlock;
> + }
> +
> ret = usb_control_msg(d->udev,
> usb_rcvctrlpipe(d->udev, 0),
> req,
> @@ -321,6 +328,7 @@ static int az6027_usb_in_op(struct dvb_usb_device *d, u8 req,
> deb_xfer("in: req. %02x, val: %04x, ind: %04x, buffer: ", req, value, index);
> debug_dump(b, blen, deb_xfer);
>
> +end_unlock:
> mutex_unlock(&d->usb_mutex);
> return ret;
> }
> @@ -330,16 +338,24 @@ static int az6027_usb_out_op(struct dvb_usb_device *d,
> u16 value,
> u16 index,
> u8 *b,
> - int blen)
> + int blen,
> + int buf_size)
> {
> int ret;
>
> - deb_xfer("out: req. %02x, val: %04x, ind: %04x, buffer: ", req, value, index);
> - debug_dump(b, blen, deb_xfer);
> -
> if (mutex_lock_interruptible(&d->usb_mutex))
> return -EAGAIN;
>
> + if (blen > buf_size) {
> + err("usb out %d bytes, but max size is %d bytes\n",
> + blen, buf_size);
> + ret = -EOPNOTSUPP;
> + goto end_unlock;
> + }
> +
> + deb_xfer("out: req. %02x, val: %04x, ind: %04x, buffer: ", req, value, index);
> + debug_dump(b, blen, deb_xfer);
> +
> ret = usb_control_msg(d->udev,
> usb_sndctrlpipe(d->udev, 0),
> req,
> @@ -350,6 +366,7 @@ static int az6027_usb_out_op(struct dvb_usb_device *d,
> blen,
> 2000);
>
> +end_unlock:
> if (ret != blen) {
> warn("usb out operation failed. (%d)", ret);
> mutex_unlock(&d->usb_mutex);
> @@ -375,7 +392,7 @@ static int az6027_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
> index = 0;
> blen = 0;
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> warn("usb out operation failed. (%d)", ret);
>
> @@ -399,7 +416,7 @@ static int az6027_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
> int az6027_power_ctrl(struct dvb_usb_device *d, int onoff)
> {
> u8 v = onoff;
> - return az6027_usb_out_op(d,0xBC,v,3,NULL,1);
> + return az6027_usb_out_op(d,0xBC,v,3,NULL,1,0);
Hmm, this will now just fail, right? Since 1 > 0. I wonder if this is a bug and the '1' should
be '0'? Or if the USB core will just ignore the length if the buffer is NULL.
I'm not familiar enough with that to be sure.
> }
> */
>
> @@ -431,7 +448,7 @@ static int az6027_ci_read_attribute_mem(struct dvb_ca_en50221 *ca,
> index = 0;
> blen = 1;
>
> - ret = az6027_usb_in_op(d, req, value, index, b, blen);
> + ret = az6027_usb_in_op(d, req, value, index, b, blen, 12);
> if (ret < 0) {
> warn("usb in operation failed. (%d)", ret);
> ret = -EINVAL;
> @@ -468,7 +485,7 @@ static int az6027_ci_write_attribute_mem(struct dvb_ca_en50221 *ca,
> index = value;
> blen = 0;
>
> - ret = az6027_usb_out_op(d, req, value1, index, NULL, blen);
> + ret = az6027_usb_out_op(d, req, value1, index, NULL, blen, 0);
> if (ret != 0)
> warn("usb out operation failed. (%d)", ret);
>
> @@ -504,7 +521,7 @@ static int az6027_ci_read_cam_control(struct dvb_ca_en50221 *ca,
> index = 0;
> blen = 2;
>
> - ret = az6027_usb_in_op(d, req, value, index, b, blen);
> + ret = az6027_usb_in_op(d, req, value, index, b, blen, 12);
> if (ret < 0) {
> warn("usb in operation failed. (%d)", ret);
> ret = -EINVAL;
> @@ -544,7 +561,7 @@ static int az6027_ci_write_cam_control(struct dvb_ca_en50221 *ca,
> index = value;
> blen = 0;
>
> - ret = az6027_usb_out_op(d, req, value1, index, NULL, blen);
> + ret = az6027_usb_out_op(d, req, value1, index, NULL, blen, 0);
> if (ret != 0) {
> warn("usb out operation failed. (%d)", ret);
> goto failed;
> @@ -575,7 +592,7 @@ static int CI_CamReady(struct dvb_ca_en50221 *ca, int slot)
> index = 0;
> blen = 1;
>
> - ret = az6027_usb_in_op(d, req, value, index, b, blen);
> + ret = az6027_usb_in_op(d, req, value, index, b, blen, 12);
> if (ret < 0) {
> warn("usb in operation failed. (%d)", ret);
> ret = -EIO;
> @@ -604,7 +621,7 @@ static int az6027_ci_slot_reset(struct dvb_ca_en50221 *ca, int slot)
> index = 0;
> blen = 0;
>
> - ret = az6027_usb_out_op(d, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(d, req, value, index, NULL, blen, 0);
> if (ret != 0) {
> warn("usb out operation failed. (%d)", ret);
> goto failed;
> @@ -616,7 +633,7 @@ static int az6027_ci_slot_reset(struct dvb_ca_en50221 *ca, int slot)
> index = 0;
> blen = 0;
>
> - ret = az6027_usb_out_op(d, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(d, req, value, index, NULL, blen, 0);
> if (ret != 0) {
> warn("usb out operation failed. (%d)", ret);
> goto failed;
> @@ -660,7 +677,7 @@ static int az6027_ci_slot_ts_enable(struct dvb_ca_en50221 *ca, int slot)
> index = 0;
> blen = 0;
>
> - ret = az6027_usb_out_op(d, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(d, req, value, index, NULL, blen, 0);
> if (ret != 0) {
> warn("usb out operation failed. (%d)", ret);
> goto failed;
> @@ -692,7 +709,7 @@ static int az6027_ci_poll_slot_status(struct dvb_ca_en50221 *ca, int slot, int o
> index = 0;
> blen = 1;
>
> - ret = az6027_usb_in_op(d, req, value, index, b, blen);
> + ret = az6027_usb_in_op(d, req, value, index, b, blen, 12);
> if (ret < 0) {
> warn("usb in operation failed. (%d)", ret);
> ret = -EIO;
> @@ -771,7 +788,7 @@ static int az6027_ci_init(struct dvb_usb_adapter *a)
> /*
> static int az6027_read_mac_addr(struct dvb_usb_device *d, u8 mac[6])
> {
> - az6027_usb_in_op(d, 0xb7, 6, 0, &mac[0], 6);
> + az6027_usb_in_op(d, 0xb7, 6, 0, &mac[0], 6, 6);
> return 0;
> }
> */
> @@ -831,7 +848,7 @@ static int az6027_frontend_poweron(struct dvb_usb_adapter *adap)
> index = 3;
> blen = 0;
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> return -EIO;
>
> @@ -851,7 +868,7 @@ static int az6027_frontend_reset(struct dvb_usb_adapter *adap)
> index = 3;
> blen = 0;
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> return -EIO;
>
> @@ -861,7 +878,7 @@ static int az6027_frontend_reset(struct dvb_usb_adapter *adap)
> blen = 0;
> msleep_interruptible(200);
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> return -EIO;
>
> @@ -872,7 +889,7 @@ static int az6027_frontend_reset(struct dvb_usb_adapter *adap)
> index = 3;
> blen = 0;
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> return -EIO;
>
> @@ -894,7 +911,7 @@ static int az6027_frontend_tsbypass(struct dvb_usb_adapter *adap, int onoff)
> index = 0;
> blen = 0;
>
> - ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen);
> + ret = az6027_usb_out_op(adap->dev, req, value, index, NULL, blen, 0);
> if (ret != 0)
> return -EIO;
>
> @@ -955,6 +972,7 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> u16 index;
> u16 value;
> int length;
> + int ret;
> u8 req;
> u8 *data;
>
> @@ -981,7 +999,12 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> }
> value = msg[i].buf[0] & 0x00ff;
> length = 1;
> - az6027_usb_out_op(d, req, value, index, data, length);
> + ret = az6027_usb_out_op(d, req, value, index,
> + data, length, 256);
> + if (ret != 0) {
> + i = ret;
> + break;
> + }
> }
>
> if (msg[i].addr == 0xd0) {
> @@ -995,7 +1018,13 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> index = (((msg[i].buf[0] << 8) & 0xff00) | (msg[i].buf[1] & 0x00ff));
> value = msg[i].addr + (msg[i].len << 8);
> length = msg[i + 1].len + 6;
> - az6027_usb_in_op(d, req, value, index, data, length);
> + ret = az6027_usb_in_op(d, req, value, index,
> + data, length, 256);
> + if (ret != 0) {
> + i = ret;
> + break;
> + }
> +
> len = msg[i + 1].len;
> for (j = 0; j < len; j++)
> msg[i + 1].buf[j] = data[j + 5];
> @@ -1013,9 +1042,12 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> value = msg[i].addr + (2 << 8);
> length = msg[i].len - 2;
> len = msg[i].len - 2;
> - for (j = 0; j < len; j++)
> - data[j] = msg[i].buf[j + 2];
> - az6027_usb_out_op(d, req, value, index, data, length);
> + ret = az6027_usb_out_op(d, req, value, index,
> + &msg[i].buf[2], length, 256);
> + if (ret != 0) {
> + i = ret;
> + break;
> + }
> }
> }
>
> @@ -1026,7 +1058,13 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> index = 0x0;
> value = msg[i].addr;
> length = msg[i].len + 6;
> - az6027_usb_in_op(d, req, value, index, data, length);
> + ret = az6027_usb_in_op(d, req, value, index,
> + data, length, 256);
> + if (ret != 0) {
> + i = ret;
> + break;
> + }
> +
> len = msg[i].len;
> for (j = 0; j < len; j++)
> msg[i].buf[j] = data[j + 5];
> @@ -1042,11 +1080,12 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> value = msg[i].addr + (1 << 8);
> length = msg[i].len - 1;
> len = msg[i].len - 1;
> -
> - for (j = 0; j < len; j++)
> - data[j] = msg[i].buf[j + 1];
> -
> - az6027_usb_out_op(d, req, value, index, data, length);
> + ret = az6027_usb_out_op(d, req, value, index,
> + &msg[i].buf[1], length, 256);
> + if (ret != 0) {
> + i = ret;
> + break;
> + }
> }
> }
> }
> --
>
Regards,
Hans
Powered by blists - more mailing lists