Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Pavel Skripkin]

On 8/24/21 3:01 PM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote:
> > Btw, not related to your patch, but I start think, that this check:
> >
> >
> > 	if (!pIo_buf) {
> > 		DBG_88E("[%s] pIo_buf == NULL\n", __func__);
> > 		status = -ENOMEM;
> > 		goto release_mutex;
> > 	}
> >
> > Should be wrapped as
> >
> > 	if (WARN_ON(unlikely(!pIo_buf)) {
> > 		...
> > 	}
> >
> > Since usb_vendor_req_buf is initialized in ->probe() and I can't see 
> > possible calltrace, which can cause zeroing this pointer.
>
> I see that usb_vendor_req_buf is initialized in rtw_init_intf_priv(). It depends on a
> kzalloc() success on allocating memory. Obviously it could fail to allocate. If it fails,
> rtw_init_intf_priv() returns _FAIL to its caller(s) (whichever they are - I didn't go too
> deep in understanding the possible calls chains).

Call chain is the most interesting part here :)

    rtw_drv_init()		<-- probe()
      usb_dvobj_init()
	rtw_init_intf_priv()

If kzalloc fails, then whole ->probe() routine fails, i.e device will be 
disconnected. There is no read() calls before rtw_init_intf_priv(), so 
if kzalloc() call was successful, there is no way how usb_vendor_req_buf 
can be NULL, since read() can happen only in case of successfully 
connected device.


Anyway, it can be NULL in case of out-of-bound write or smth else, but 
there is no explicit usb_alloc_vendor_req_buf = NULL in this driver.
We should complain about completely wrong driver behavior, IMO :)


Does it make sense?



With regards,
Pavel Skripkin