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

[Pavel Skripkin <paskripkin@gmail.com>]

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