2017-11-27 20:40 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>: > Am 27.11.2017 um 13:33 schrieb Bartosz Golaszewski: >> 2017-11-24 7:47 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>: >>> 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@xxxxxxxxx> >>> --- >>> 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
