Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32

[Phillip Potter]

On Sun, 22 Aug 2021 at 15:36, Pavel Skripkin <paskripkin@xxxxxxxxx> wrote:
> -static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> +static int usb_read32(struct intf_hdl *pintfhdl, u32 addr, u32 *data)
>  {
>         u8 requesttype;
>         u16 wvalue;
>         u16 len;
> -       __le32 data;
> +       int res;
> +       __le32 tmp;
> +
> +       if (WARN_ON(unlikely(!data)))
> +               return -EINVAL;
>
>         requesttype = 0x01;/* read_in */
>
>         wvalue = (u16)(addr & 0x0000ffff);
>         len = 4;
>
> -       usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> +       res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> +       if (res < 0) {
> +               dev_err(dvobj_to_dev(pintfhdl->pintf_dev), "Failed to read 32 bytes: %d\n", res);
> +       } else {
> +               /* Noone cares about positive return value */
> +               *data = le32_to_cpu(tmp);
> +               res = 0;
> +       }
>
> -       return le32_to_cpu(data);
> +       return res;
>  }

Dear Pavel,

OK, found the issue with decoded stack trace after reviewing this
usb_read32 function. Your line:
res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);

should read:
res = usbctrl_vendorreq(pintfhdl, wvalue, &tmp, len, requesttype);

With this change, the driver runs fine with no crashes/oopses. I will
explain the issue but you can probably see already, so I hope I'm not
coming across as patronising, just trying to be helpful :-)

Essentially, you are taking the address of the data function parameter
on this line with &data, a pointer to u32, which is giving you a
pointer to a pointer to u32 (u32 **) for this function parameter
variable. When passed to usbctrl_vendorreq, it is being passed to
memcpy inside this function as a void *, meaning that memcpy
subsequently overwrites the value of the memory address inside data to
point to a different location, which is problem when it is later
deferenced at:
*data = le32_to_cpu(tmp);
causing the OOPS

Also, as written, you can probably see that tmp is uninitialised. This
looks like a typo, so guessing this wasn't your intention. Anyhow,
with that small change, usbctrl_vendorreq reads into tmp, which is
then passed to le32_to_cpu whose return value is stored via the
deferenced data ptr (which now has its original address within and not
inadvertently modified). Hope this helps, and I'd be happy to Ack the
series if you want to resend this patch. Many thanks.

Regards,
Phil