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

Pavel Skripkin
2021-08-24T12:09:22Z

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