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

[Greg KH]

On Sun, Aug 22, 2021 at 03:10:56PM +0300, Pavel Skripkin wrote:
> 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

That's not a good design, it should be fixed.  Note there is the new
usb_control_msg_recv() function which should probably be used instead
here, to prevent this problem from happening.

> > 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?

Relax, there is no need to get jumpy, people do not have to respond
instantly to emails here.  Especially when it is not their job to do so.

greg k-h