Messages in this thread | | | Date | Tue, 4 Sep 2018 09:26:30 +0200 | From | Quentin Schulz <> | Subject | Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32 |
| |
Hi Andrew,
On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote: > > Just to be sure, we're talking here about making sure the value stored > > in the DT is not bigger than the specified value (here an u8)? If so, > > that isn't the reason why I'm suggesting those two patches. > > > > Without /bits 8/ in the DT property, whatever were the values I put in > > the property, I'd always get a 0. So I need to fix it either in the DT > > (but Rob does not really like it) or in the driver. > > Hi Quentin > > Ah, you are fixing endian issues. That was not clear to me from the > commit message. > > I don't know enough about how DT stores values in the blob. Is there > type info? Can the DT core tell if a value in the blob is a u8 or a > u32? It would be nice if it warned about reading a u8 from a u32 > blob. >
From my quick research, the lower bound checking is performed by of_property_read_u* functions but not the higher bound checking (the internal function of_find_property_value_of_size allows higher bound checking but it seems it's never used by those functions (see 0 in sz_max of of_property_read_variable_u*_array)).
sz_max is used by of_property_read_variable_u*_array to copy at a maximum of sz_max values in the output buffer. If sz_max is 0, it takes sz_min so it's an array of definite size. So since sz_max is 0 for all calls to of_property_read_variable_u*_array by of_property_read_u*_array, we basically know we'll get a buffer of sz_min values but we don't actually make use of the higher bound checking of of_find_property_value_of_size.
We could enforce this higher bound check by, instead of setting sz_max to 0, setting sz_max to sz_min in calls to of_property_read_u*_array.
But I guess there is a reason for sz_max being 0. Rob, Richard (commit signer of this code) do you know why? Could you explain?
> Anyway, this change still removes some bounds checking. Are they > important? Do they need to be added back? >
The edge-slowdown and the vddmac values are compared against a const array so we´re fine with those ones.
For the led-X-mode, I added a constant for supported modes that gets checked when retrieving the DT property. So we´re fine here too.
Quentin [unhandled content-type:application/pgp-signature] | |