[PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Hans Verkuil]

The memcpy in gl861_i2c_read_ex() in gl861.c swapped the src and dst arguments,
leaving the rbuf uninitialized.

This issue caused this syzbot error:

https://syzkaller.appspot.com/bug?extid=9e6bf7282557bd1fc80d

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-and-tested-by: syzbot+9e6bf7282557bd1fc80d@syzkaller.appspotmail.com
Fixes: commit b30cc07de8a9 ("media: dvb-usb/friio, dvb-usb-v2/gl861: decompose friio and merge with gl861")
---
Does anyone have this hardware? This device must have been dead for about
a year, ever since commit b30cc07de8a9 was merged.
---
diff --git a/drivers/media/usb/dvb-usb-v2/gl861.c b/drivers/media/usb/dvb-usb-v2/gl861.c
index b784d9da1a82..65d7c51ef56f 100644
--- a/drivers/media/usb/dvb-usb-v2/gl861.c
+++ b/drivers/media/usb/dvb-usb-v2/gl861.c
@@ -222,7 +222,7 @@ gl861_i2c_read_ex(struct dvb_usb_device *d, u8 addr, u8 *rbuf, u16 rlen)
 				 GL861_REQ_I2C_READ, GL861_READ,
 				 addr << (8 + 1), 0x0100, buf, rlen, 2000);
 	if (ret > 0 && rlen > 0)
-		memcpy(buf, rbuf, rlen);
+		memcpy(rbuf, buf, rlen);
 	kfree(buf);
 	return ret;
 }

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Akihiro TSUKADA]

> Does anyone have this hardware? This device must have been dead for about
> a year, ever since commit b30cc07de8a9 was merged.

I have one. (and I wrote the patch).
Since I do not use it regularly and
my app did not use the return value meaningfully,
I have not noticed.

regards,
Akihiro

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Mauro Carvalho Chehab]

Akihiro-san,

Em Fri, 16 Aug 2019 10:04:59 +0900
Akihiro TSUKADA <tskd08@gmail.com> escreveu:

> > Does anyone have this hardware? This device must have been dead for about
> > a year, ever since commit b30cc07de8a9 was merged.  
> 
> I have one. (and I wrote the patch).
> Since I do not use it regularly and
> my app did not use the return value meaningfully,
> I have not noticed.

Could you please test the patch and check if the return results are
now consistent and that it won't break anything?

Also, please send us a Reviewed-by and/or Tested-by.


Thanks,
Mauro

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Akihiro TSUKADA]

> Could you please test the patch and check if the return results are
> now consistent and that it won't break anything?

I have tested the patch and it worked without problems.

Testd-by: Akihiro Tsukada <tskd08@gmail.com>

I could not noticed the bug because
the device was registered without any error messages,
and it seemed to work even with the bug.
(Though actually I was wrong and missed that
 the device does not work after reboot or re-plugging).

After applying this patch, I have confirmed that the device
now works after reboot/re-plugging without any problems.

note:
The patched func: gl861_i2c_read_ex was used in device's early init,
called from d->props->power_ctrl (from dvb_usbv2_init).
But dvb_usbv2_init does not check the return value of it,
and if the device had been initialized previously
it can work even with the interrupted init process in power_ctrl().

--
Akihiro

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Antti Palosaari]

On 8/17/19 4:22 PM, Akihiro TSUKADA wrote:
>> Could you please test the patch and check if the return results are
>> now consistent and that it won't break anything?
> 
> I have tested the patch and it worked without problems.
> 
> Testd-by: Akihiro Tsukada <tskd08@gmail.com>
> 
> I could not noticed the bug because
> the device was registered without any error messages,
> and it seemed to work even with the bug.
> (Though actually I was wrong and missed that
>   the device does not work after reboot or re-plugging).
> 
> After applying this patch, I have confirmed that the device
> now works after reboot/re-plugging without any problems.
> 
> note:
> The patched func: gl861_i2c_read_ex was used in device's early init,
> called from d->props->power_ctrl (from dvb_usbv2_init).
> But dvb_usbv2_init does not check the return value of it,
> and if the device had been initialized previously
> it can work even with the interrupted init process in power_ctrl().


I suspect all whole friio_reset() function is not needed as it has 
worked even I/O has been broken.

Also tuner I2C adapter is implemented wrong (I think I mentioned that 
earlier). As tuner sits behind demod I2C-adapter/gate that whole logic 
should be on demod driver.

regards
Antti



-- 
http://palosaari.fi/

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Akihiro TSUKADA]

Hi,

> I suspect all whole friio_reset() function is not needed as it has
> worked even I/O has been broken.

It worked because the old driver
(that I rmmod'ed before installing the testing driver)
properly init'ed the device.
If I re-plug it (or reboot), it does not work.
So it is needed.

> Also tuner I2C adapter is implemented wrong (I think I mentioned that
> earlier). As tuner sits behind demod I2C-adapter/gate that whole logic
> should be on demod driver.

But according to USB packet capture logs of the windows version,
it makes eccentric use of USB messages ('bRequest' field),
that (I believe) necessitates the current implementation,
as I mentioned in the past thread.

Regards,
Akihiro

Re: [PATCH] dvb-usb-v2/gl861: fix wrong memcpy

[Antti Palosaari]

On 8/22/19 5:00 AM, Akihiro TSUKADA wrote:
> Hi,
> 
>> I suspect all whole friio_reset() function is not needed as it has
>> worked even I/O has been broken.
> 
> It worked because the old driver
> (that I rmmod'ed before installing the testing driver)
> properly init'ed the device.
> If I re-plug it (or reboot), it does not work.
> So it is needed.
> 
>> Also tuner I2C adapter is implemented wrong (I think I mentioned that
>> earlier). As tuner sits behind demod I2C-adapter/gate that whole logic
>> should be on demod driver.
> 
> But according to USB packet capture logs of the windows version,
> it makes eccentric use of USB messages ('bRequest' field),
> that (I believe) necessitates the current implementation,
> as I mentioned in the past thread.

That is because it has 2 i2c write methods - one using only 
usb_control_msg() header and other header + payload data. When 1 or 2 
byte long i2c message is send it is wise to use only "header" to reduce 
IO as it could carry needed data.

Anyhow, I will send patch soon which adds needed logic to i2c adapter. 
Then it is easier to understand.

regards
Antti


-- 
http://palosaari.fi/