Re: [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs

[Pavel Skripkin]

On 8/22/21 1:59 PM, Fabio M. De Francesco wrote:
> On Sunday, August 22, 2021 12:09:29 PM CEST Pavel Skripkin wrote:
> > 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;
> > 	}
> > }
>
> It's up to the callers of _rtw_usb*() to check return values and then act
> accordingly.
>
> It doesn't matter whether or not *pdata is initialized because usb_read*()
> returns data = 0 if usb_control_msg() has not initialized/changed  its third
> parameter. Then _rtw_read*() receive 0 or initialized data depending on errors
> or no errors. Finally _rtw_read*() returns that same value to the callers (via
> r_val).
>
> So, it's up to the callers to test if (!_rtw_read*()) and then act
> accordingly. If they get 0 they should know how to handle the errors.

Yes, but _rtw_read*() == 0 indicates 2 states:

	1. Error on transfer side
	2. Actual register value is 0

> Furthermore, we have already either adapt->bSurpriseRemoved = true or haldata-
> > srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL. Depending on contexts 
> where _rtw_read*() are called, perhaps they could also check the two variables
> above.

Yes, Wifi_Error_Status can be used, but it's set every time an error 
occurred. For example if 8th usb_control_msg() was successful 
Wifi_Error_Status will be set to error anyway. It's can be easily fixed, 
of course.

IMO, we should switch to standard way of handling these type of errors 
to move the driver out of staging someday


BTW: syzbot already found uninit value bug in r817xu driver:

https://syzkaller.appspot.com/bug?id=3cd92b1d85428b128503bfa7a250294c9ae00bd8

The usb related code in these drivers is the same, so bugs I am talking 
about are real.

> In summation. if anything should be changed, it is the code of the callers of
> _rtw_read*() if you find out they they don't properly handle the returning
> values of this function. You should find every place where _rtw_read*() are
> called and figure out if the returns are properly checked and handled; if not,
> make some change only there.
>
> Larry, Philip, where are you? Am I missing something?

I am waiting for their replies too :) I have almost ready v2, so...



With regards,
Pavel Skripkin