On Tue, 24 Aug 2021 at 09:47, Pavel Skripkin [off-list ref] wrote:

On 8/24/21 11:38 AM, Fabio M. De Francesco wrote:

quoted

On Tuesday, August 24, 2021 8:40:18 AM CEST Pavel Skripkin wrote:

quoted

On 8/24/21 3:10 AM, Fabio M. De Francesco wrote:

quoted

On Tuesday, August 24, 2021 1:33:46 AM CEST Phillip Potter wrote:

quoted

On Sun, 22 Aug 2021 at 15:36, Pavel Skripkin [off-list ref] wrote:

quoted

-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);

Dear Philip,

No, it should read:

res = usbctrl_vendorreq(pintfhdl, wvalue, data, len, requesttype);

I suspect that Pavel didn't notice he was reusing a line of the old code
wth no due changes.

quoted

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.

I think that another typo is having 'tmp', because that variable is unnecessary
and "*data = le32_to_cpu(tmp);" is wrong too.

Now I also see that also usb_read16() is wrong, while usb_read8() (the one that
I had read yesterday) is the only correct function of the three usb_read*().

Hi, guys!


Sorry for breaking your system, Phillip. This code was part of "last
minute" changes and yes, it's broken :)

I get what Phillip said, because I _should_ read into tmp variable
instead of directly to data, but I don't get Fabio's idea, sorry.

Hi Pavel,

I (wrongly?) assumed from the prototype of usb_read32() that u32 *data is in native
endianness. So, I didn't see the necessity of using _le32 tmp and then convert that tmp
with le32_to_cpu().

I simply thought that data could be passed to usbctrl_vendorreq as it-is.

quoted

Data from chip comes in little-endian, so we _should_ convert it to
cpu's endian. Temp variable is needed to make smatch and all other
static anylis tools happy about this code.

Now that you explained that "Data from chip comes in little-endian", obviously
I must agree with you that the code needs tmp and that tmp must be
swapped by le32_to_cpu(), ahead of assigning it to *data.

Just a curiosity... Since I was not able to see that *data is returned in little endian,
can you please point me where in the code you found out that it is? There must
be some place in the code that I'm unable to find and see that *data is LE.

Thanks in advance,

Fabio

Hi, Fabio!

previous usb_read16() realization, which is 100% right:


static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
{
        u8 requesttype;
        u16 wvalue;
        u16 len;
        __le32 data;

        requesttype = 0x01;/* read_in */
        wvalue = (u16)(addr & 0x0000ffff);
        len = 2;
        usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);

        return (u16)(le32_to_cpu(data) & 0xffff);
}


Bases on this code, I think, it's oblivious, that data comes in
little-endian. That's why I leaved temp variable for casting le32 to
cpu's endianess.

I could just read into u{16,32} * and then make smth like

*data = le32_to_cpu(*data)

but static analysis tools will complain about wrong data type passed to
  le32_to_cpu()

+ Phillip tested fixed v2 version and it worked well for him. I guess,
Phillip was able to spot weird driver behavior, if this cast is wrong.




With regards,
Pavel Skripkin

In my mind we can't necessarily assume we are running on a little
endian CPU, even if we probably are for practical purposes. That's why
my fix looked how it did, but I'm happy to be corrected :-) Also, I
can see Dan has looked at the code with suggestions as well. I know
you have published v3 - sorry, not had time to review/test it yet.

Regards,
Phil