On 8/22/21 12:53 PM, Fabio M. De Francesco wrote:
On Friday, August 20, 2021 7:07:28 PM CEST Pavel Skripkin wrote:
Hi, Greg, Larry and Phillip!
I noticed, that new staging driver was added like 3 weeks ago and I decided
to look at the code, because drivers in staging directory are always buggy.
The first thing I noticed is *no one* was checking read operations result,
but
it can fail and driver may start writing random stack values into registers.
It
can cause driver misbehavior or device misbehavior.
After the messages I wrote yesterday, I had some minutes to look deeper at the
code that would be changed by these patches.
I think that it does not look like that the driver could return "random stack
values into registers" and I think this entire series in unnecessary.
As far as I understand this driver (though I must admit that I really don't
know how to write drivers, and I'm not interested in understanding - at the
moment, at least), all the usb_read*() call usbctrl_vendorreq() and the latter
*does* proper error checking before returning to the callers the read data.
Please, look at the code copied from usbctrl_vendorreq() and pasted here (some
comments are mine):
/* start of code */
static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void
*pdata, u16 len, u8 requesttype)
{
/* test if everything is OK for transfers and setup the necessary variables */
[...]
status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
reqtype, value,
REALTEK_USB_VENQT_CMD_IDX,
pIo_buf, len,
RTW_USB_CONTROL_MSG_TIMEOUT);
if (status == len) { /* Success this control transfer. */
rtw_reset_continual_urb_error(dvobjpriv);
if (requesttype == 0x01)
memcpy(pdata, pIo_buf, len); /* pdata
receives the read data */
} else { /* error cases */
[...]
}
/* end of code */
So, *I cannot ack this RFC*, unless maintainers say I'm missing something.
Larry, Philip, since you have much more knowledge than me about r8188eu (and,
more in general, on device drivers) may you please say what you think about my
arguments against this series?
Hi, Fabio!
Thank you for looking into this, but I still can see the case when pdata
won't be initialized:
pdata is initialized only in case of successful transfer, i.e len > 0.
It means some data was received (maybe not full length, but anyway). In
case of usb_control_msg() error (for example -ENOMEM) code only does
this code block:
if (status < 0) {
if (status == (-ESHUTDOWN) || status == -ENODEV) {
adapt->bSurpriseRemoved = true;
} else {
struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
}
}
And then just loops further. In case of 10 ENOMEM in a row,. passed
pdata won't be initialized at all and driver doesn't do anything about
it. I believe, it's not good approach to play with random values. We
should somehow handle transfer errors all across the driver.
If I am missing something, please, let me know :)
With regards,
Pavel Skripkin
