Re: [PATCH] eeprom: at24: check at24_read/write arguments

[Heiner Kallweit <hkallweit1@gmail.com>]

Am 27.11.2017 um 13:33 schrieb Bartosz Golaszewski:
> 2017-11-24 7:47 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> So far we completely rely on the caller to provide valid arguments.
>> To be on the safe side perform an own sanity check.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/misc/eeprom/at24.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 00d602be7..52cbaeb6f 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -569,6 +569,9 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
>>         if (unlikely(!count))
>>                 return count;
>>
>> +       if (off + count > at24->chip.byte_len)
>> +               return -EINVAL;
>> +
>>         client = at24_translate_offset(at24, &off);
>>
>>         ret = pm_runtime_get_sync(&client->dev);
>> @@ -614,6 +617,9 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
>>         if (unlikely(!count))
>>                 return -EINVAL;
>>
>> +       if (off + count > at24->chip.byte_len)
>> +               return -EINVAL;
>> +
>>         client = at24_translate_offset(at24, &off);
>>
>>         ret = pm_runtime_get_sync(&client->dev);
>> --
>> 2.15.0
>>
>>
> 
> Out of curiosity: have you tried what happens currently if we try to
> read past the size of the nvmem device size?
> 
When reading moderately past the end on most chips nothing bad happens.
But if you look at at24_translate_offset: if the offset is big enough 
then i becomes too big and at24->client[i] accesses invalid memory.

at24_read/write are used by the nvmem core only. And the nvmem sysfs
interface checks the parameters good enough. However thare are few
nvmem API functions not doing any parameter check,
e.g. nvmem_device_read.

Best solution would be if nvmem core guarantees that all calls to
the nvmem provider read/write callbacks are done with valid
parameters only. At least as long as this is not the case I'd suggest
to check on our side too.

The decision to apply this patch or not has an impact on my other
patch series due to needed rebasing.
For now I'll send the next version of my series assuming that this
patch will be applied.

> Thanks,
> Bartosz
>