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

[Bartosz Golaszewski <brgl@bgdev.pl>]

2017-11-27 20:40 GMT+01:00 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.
>

Oh, it will be applied alright, I was just wondering if it has any
actual impact with current kernel. I mostly worried about user space
accesses, but I see we'd hit EOF anyway before reading past the
eeprom.

Feel free to rebase on top of this commit.

Thanks,
Bartosz