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
