| Message ID | 98cb0a1c-0db8-951b-80e1-12756678db9a@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dvb-usb-v2/gl861: fix wrong memcpy | expand |
> 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
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
> 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
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
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
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
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; }
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. ---