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

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

[Heiner Kallweit]

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

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

[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?

Thanks,
Bartosz

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

[Heiner Kallweit]

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
> 

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

[Bartosz Golaszewski]

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

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

[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
>
>

Applied to at24/fixes, thanks!

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]


For the record:

> Best solution would be if nvmem core guarantees that all calls to
> the nvmem provider read/write callbacks are done with valid
> parameters only.

+1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]