From: Wei Chen <[email protected]>
In ec168_i2c_xfer, msg is controlled by user. When msg[i].buf is null
and msg[i].len is zero, former checks on msg[i].buf would be passed.
If accessing msg[i].buf[0] without sanity check, null pointer deref
would happen. We add check on msg[i].len to prevent crash.
Similar commit:
commit 0ed554fd769a ("media: dvb-usb: az6027: fix null-ptr-deref in az6027_i2c_xfer()")
Link: https://lore.kernel.org/linux-media/[email protected]
Signed-off-by: Wei Chen <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/usb/dvb-usb-v2/ec168.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/media/usb/dvb-usb-v2/ec168.c b/drivers/media/usb/dvb-usb-v2/ec168.c
index 7ed0ab9e429b..0e4773fc025c 100644
--- a/drivers/media/usb/dvb-usb-v2/ec168.c
+++ b/drivers/media/usb/dvb-usb-v2/ec168.c
@@ -115,6 +115,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
while (i < num) {
if (num > i + 1 && (msg[i+1].flags & I2C_M_RD)) {
if (msg[i].addr == ec168_ec100_config.demod_address) {
+ if (msg[i].len < 1) {
+ i = -EOPNOTSUPP;
+ break;
+ }
req.cmd = READ_DEMOD;
req.value = 0;
req.index = 0xff00 + msg[i].buf[0]; /* reg */
@@ -131,6 +135,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
}
} else {
if (msg[i].addr == ec168_ec100_config.demod_address) {
+ if (msg[i].len < 1) {
+ i = -EOPNOTSUPP;
+ break;
+ }
req.cmd = WRITE_DEMOD;
req.value = msg[i].buf[1]; /* val */
req.index = 0xff00 + msg[i].buf[0]; /* reg */
@@ -139,6 +147,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
ret = ec168_ctrl_msg(d, &req);
i += 1;
} else {
+ if (msg[i].len < 1) {
+ i = -EOPNOTSUPP;
+ break;
+ }
req.cmd = WRITE_I2C;
req.value = msg[i].buf[0]; /* val */
req.index = 0x0100 + msg[i].addr; /* I2C addr */
--
2.40.1
Am 13.05.23 um 19:57 schrieb Mauro Carvalho Chehab:
> From: Wei Chen <[email protected]>
>
> In ec168_i2c_xfer, msg is controlled by user. When msg[i].buf is null
> and msg[i].len is zero, former checks on msg[i].buf would be passed.
> If accessing msg[i].buf[0] without sanity check, null pointer deref
> would happen. We add check on msg[i].len to prevent crash.
>
> Similar commit:
> commit 0ed554fd769a ("media: dvb-usb: az6027: fix null-ptr-deref in az6027_i2c_xfer()")
>
Review comment below.
> Link: https://lore.kernel.org/linux-media/[email protected]
> Signed-off-by: Wei Chen <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/usb/dvb-usb-v2/ec168.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/ec168.c b/drivers/media/usb/dvb-usb-v2/ec168.c
> index 7ed0ab9e429b..0e4773fc025c 100644
> --- a/drivers/media/usb/dvb-usb-v2/ec168.c
> +++ b/drivers/media/usb/dvb-usb-v2/ec168.c
> @@ -115,6 +115,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
> while (i < num) {
> if (num > i + 1 && (msg[i+1].flags & I2C_M_RD)) {
> if (msg[i].addr == ec168_ec100_config.demod_address) {
> + if (msg[i].len < 1) {
> + i = -EOPNOTSUPP;
> + break;
> + }
> req.cmd = READ_DEMOD;
> req.value = 0;
> req.index = 0xff00 + msg[i].buf[0]; /* reg */
> @@ -131,6 +135,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
> }
> } else {
> if (msg[i].addr == ec168_ec100_config.demod_address) {
> + if (msg[i].len < 1) {
> + i = -EOPNOTSUPP;
> + break;
> + }
The check condition should be msg[i].len < 2 or != 2. The following
lines access msg[i].buf elements 0 and 1.
> req.cmd = WRITE_DEMOD;
> req.value = msg[i].buf[1]; /* val */
> req.index = 0xff00 + msg[i].buf[0]; /* reg */
> @@ -139,6 +147,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
> ret = ec168_ctrl_msg(d, &req);
> i += 1;
> } else {
> + if (msg[i].len < 1) {
> + i = -EOPNOTSUPP;
> + break;
> + }
> req.cmd = WRITE_I2C;
> req.value = msg[i].buf[0]; /* val */
> req.index = 0x0100 + msg[i].addr; /* I2C addr */
Dear Linux Developers,
Thank you for the review for my patch in driver az6027 and ec168.
Yes, I agree with you. Sorry for my mistake. Since these two patches has already been accepted and merged in git tree media, should I send a new patch to revise this problem? Or how could I revise an accepted patch?
Thanks,
Wei
------ Original Message ------
From [email protected]
To "Mauro Carvalho Chehab" <[email protected]>
Cc "Wei Chen" <[email protected]>; "Antti Palosaari" <[email protected]>; [email protected]; [email protected]
Date 2023/5/17 13:10:34
Subject Re: [PATCH 05/24] media: dvb-usb-v2: ec168: fix null-ptr-deref in ec168_i2c_xfer()
>Am 13.05.23 um 19:57 schrieb Mauro Carvalho Chehab:
>>From: Wei Chen <[email protected]>
>>
>>In ec168_i2c_xfer, msg is controlled by user. When msg[i].buf is null
>>and msg[i].len is zero, former checks on msg[i].buf would be passed.
>>If accessing msg[i].buf[0] without sanity check, null pointer deref
>>would happen. We add check on msg[i].len to prevent crash.
>>
>>Similar commit:
>>commit 0ed554fd769a ("media: dvb-usb: az6027: fix null-ptr-deref in az6027_i2c_xfer()")
>>
>Review comment below.
>
>>Link: https://lore.kernel.org/linux-media/[email protected]
>>Signed-off-by: Wei Chen <[email protected]>
>>Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>>---
>> drivers/media/usb/dvb-usb-v2/ec168.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>>diff --git a/drivers/media/usb/dvb-usb-v2/ec168.c b/drivers/media/usb/dvb-usb-v2/ec168.c
>>index 7ed0ab9e429b..0e4773fc025c 100644
>>--- a/drivers/media/usb/dvb-usb-v2/ec168.c
>>+++ b/drivers/media/usb/dvb-usb-v2/ec168.c
>>@@ -115,6 +115,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
>> while (i < num) {
>> if (num > i + 1 && (msg[i+1].flags & I2C_M_RD)) {
>> if (msg[i].addr == ec168_ec100_config.demod_address) {
>>+ if (msg[i].len < 1) {
>>+ i = -EOPNOTSUPP;
>>+ break;
>>+ }
>> req.cmd = READ_DEMOD;
>> req.value = 0;
>> req.index = 0xff00 + msg[i].buf[0]; /* reg */
>>@@ -131,6 +135,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
>> }
>> } else {
>> if (msg[i].addr == ec168_ec100_config.demod_address) {
>>+ if (msg[i].len < 1) {
>>+ i = -EOPNOTSUPP;
>>+ break;
>>+ }
>The check condition should be msg[i].len < 2 or != 2. The following lines access msg[i].buf elements 0 and 1.
>> req.cmd = WRITE_DEMOD;
>> req.value = msg[i].buf[1]; /* val */
>> req.index = 0xff00 + msg[i].buf[0]; /* reg */
>>@@ -139,6 +147,10 @@ static int ec168_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
>> ret = ec168_ctrl_msg(d, &req);
>> i += 1;
>> } else {
>>+ if (msg[i].len < 1) {
>>+ i = -EOPNOTSUPP;
>>+ break;
>>+ }
>> req.cmd = WRITE_I2C;
>> req.value = msg[i].buf[0]; /* val */
>> req.index = 0x0100 + msg[i].addr; /* I2C addr */
>
Am 19.05.23 um 11:17 schrieb Wei Chen:
> Dear Linux Developers,
>
Hi Wei,
> Thank you for the review for my patch in driver az6027 and ec168.
>
> Yes, I agree with you. Sorry for my mistake. Since these two patches has
> already been accepted and merged in git tree media, should I send a new
> patch to revise this problem? Or how could I revise an accepted patch?
I think the best is to send new patches on top of the already accepted
patches.
Regards
Matthias
>
> Thanks,
> Wei
>
>
> ------ Original Message ------
>> From [email protected]
> To "Mauro Carvalho Chehab" <[email protected]>
> Cc "Wei Chen" <[email protected]>; "Antti Palosaari"
> <[email protected]>; [email protected]; [email protected]
> Date 2023/5/17 13:10:34
> Subject Re: [PATCH 05/24] media: dvb-usb-v2: ec168: fix null-ptr-deref
> in ec168_i2c_xfer()
>
>> Am 13.05.23 um 19:57 schrieb Mauro Carvalho Chehab:
>>> From: Wei Chen <[email protected]>
>>>
>>> In ec168_i2c_xfer, msg is controlled by user. When msg[i].buf is null
>>> and msg[i].len is zero, former checks on msg[i].buf would be passed.
>>> If accessing msg[i].buf[0] without sanity check, null pointer deref
>>> would happen. We add check on msg[i].len to prevent crash.
>>>
>>> Similar commit:
>>> commit 0ed554fd769a ("media: dvb-usb: az6027: fix null-ptr-deref in
>>> az6027_i2c_xfer()")
>>>
>> Review comment below.
>>
>>> Link:
>>> https://lore.kernel.org/linux-media/[email protected]
>>> Signed-off-by: Wei Chen <[email protected]>
>>> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>>> ---
>>> drivers/media/usb/dvb-usb-v2/ec168.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/dvb-usb-v2/ec168.c
>>> b/drivers/media/usb/dvb-usb-v2/ec168.c
>>> index 7ed0ab9e429b..0e4773fc025c 100644
>>> --- a/drivers/media/usb/dvb-usb-v2/ec168.c
>>> +++ b/drivers/media/usb/dvb-usb-v2/ec168.c
>>> @@ -115,6 +115,10 @@ static int ec168_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg msg[],
>>> while (i < num) {
>>> if (num > i + 1 && (msg[i+1].flags & I2C_M_RD)) {
>>> if (msg[i].addr == ec168_ec100_config.demod_address) {
>>> + if (msg[i].len < 1) {
>>> + i = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> req.cmd = READ_DEMOD;
>>> req.value = 0;
>>> req.index = 0xff00 + msg[i].buf[0]; /* reg */
>>> @@ -131,6 +135,10 @@ static int ec168_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg msg[],
>>> }
>>> } else {
>>> if (msg[i].addr == ec168_ec100_config.demod_address) {
>>> + if (msg[i].len < 1) {
>>> + i = -EOPNOTSUPP;
>>> + break;
>>> + }
>> The check condition should be msg[i].len < 2 or != 2. The following
>> lines access msg[i].buf elements 0 and 1.
>>> req.cmd = WRITE_DEMOD;
>>> req.value = msg[i].buf[1]; /* val */
>>> req.index = 0xff00 + msg[i].buf[0]; /* reg */
>>> @@ -139,6 +147,10 @@ static int ec168_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg msg[],
>>> ret = ec168_ctrl_msg(d, &req);
>>> i += 1;
>>> } else {
>>> + if (msg[i].len < 1) {
>>> + i = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> req.cmd = WRITE_I2C;
>>> req.value = msg[i].buf[0]; /* val */
>>> req.index = 0x0100 + msg[i].addr; /* I2C addr */
>>