Message ID | 1544520254-4210-1-git-send-email-biju.das@bp.renesas.com (mailing list archive) |
---|---|
State | Not Applicable |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | [v3] nvmem: check invalid number of bytes in nvmem_{read,write}() | expand |
After applying this patch I get below errors with W=1 C=1 On 11/12/18 09:24, Biju Das wrote: > + if (unlikely(check_add_overflow(bytes, offset, &new_bytes))) > + return -EOVERFLOW; > drivers/nvmem/core.c:82:13: error: incompatible types in comparison expression (different type sizes) drivers/nvmem/core.c:82:13: error: incompatible types in comparison expression (different type sizes) drivers/nvmem/core.c:113:13: error: incompatible types in comparison expression (different type sizes) drivers/nvmem/core.c:113:13: error: incompatible types in comparison expression (different type sizes) --srini
Hello Srini, Thanks for the feedback. > Subject: Re: [PATCH v3] nvmem: check invalid number of bytes in > nvmem_{read,write}() > > > > After applying this patch I get below errors with W=1 C=1 > > On 11/12/18 09:24, Biju Das wrote: > > +if (unlikely(check_add_overflow(bytes, offset, &new_bytes))) > > +return -EOVERFLOW; > > drivers/nvmem/core.c:82:13: error: incompatible types in comparison > expression (different type sizes) > drivers/nvmem/core.c:82:13: error: incompatible types in comparison > expression (different type sizes) > drivers/nvmem/core.c:113:13: error: incompatible types in comparison > expression (different type sizes) > drivers/nvmem/core.c:113:13: error: incompatible types in comparison > expression (different type sizes) I was trying with arm32 toolchain and compiler happy. Now tried with Arm64 toolchain, it provides a warning and the below typecast fixed the issue. if (unlikely(check_add_overflow(bytes, (size_t)offset, &new_bytes))) Does typecasting to (size_t) fixed the issue in your environment? Please let me know. regards, Biju [https://www2.renesas.eu/media/email/unicef.jpg] This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world. We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year. Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hi Biju, On Tue, Dec 11, 2018 at 11:55 AM Biju Das <biju.das@bp.renesas.com> wrote: > > Subject: Re: [PATCH v3] nvmem: check invalid number of bytes in > > nvmem_{read,write}() > > > > After applying this patch I get below errors with W=1 C=1 > > > > On 11/12/18 09:24, Biju Das wrote: > > > +if (unlikely(check_add_overflow(bytes, offset, &new_bytes))) > > > +return -EOVERFLOW; > > > drivers/nvmem/core.c:82:13: error: incompatible types in comparison > > expression (different type sizes) > > drivers/nvmem/core.c:82:13: error: incompatible types in comparison > > expression (different type sizes) > > drivers/nvmem/core.c:113:13: error: incompatible types in comparison > > expression (different type sizes) > > drivers/nvmem/core.c:113:13: error: incompatible types in comparison > > expression (different type sizes) > > I was trying with arm32 toolchain and compiler happy. Now tried with Arm64 toolchain, it provides a warning and the below typecast fixed the issue. > if (unlikely(check_add_overflow(bytes, (size_t)offset, &new_bytes))) > > Does typecasting to (size_t) fixed the issue in your environment? Please let me know. That's a side-effect of offset not being loff_t... I think that should be fixed first, else you will forget about removing the cast later ("casts are evil"). Gr{oetje,eeting}s, Geert
On 11/12/18 11:06, Geert Uytterhoeven wrote: >> I was trying with arm32 toolchain and compiler happy. Now tried with Arm64 toolchain, it provides a warning and the below typecast fixed the issue. >> if (unlikely(check_add_overflow(bytes, (size_t)offset, &new_bytes))) >> >> Does typecasting to (size_t) fixed the issue in your environment? Please let me know. > That's a side-effect of offset not being loff_t... check_add_overflow will expect all the params to be of same type, so changing to loff_t will not help too. > I think that should be fixed first, else you will forget about > removing the cast later Yes, I agree. > ("casts are evil").
Hi Srinivas, On Tue, Dec 11, 2018 at 12:10 PM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 11/12/18 11:06, Geert Uytterhoeven wrote: > >> I was trying with arm32 toolchain and compiler happy. Now tried with Arm64 toolchain, it provides a warning and the below typecast fixed the issue. > >> if (unlikely(check_add_overflow(bytes, (size_t)offset, &new_bytes))) > >> > >> Does typecasting to (size_t) fixed the issue in your environment? Please let me know. > > That's a side-effect of offset not being loff_t... > check_add_overflow will expect all the params to be of same type, so > changing to loff_t will not help too. Right, it has separate checks for signed/unsigned additions, but both parameter types must have the same signedness, too. Gr{oetje,eeting}s, Geert
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index f7301bb..dfbc8ff 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -76,19 +76,60 @@ static struct lock_class_key eeprom_lock_key; static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, void *val, size_t bytes) { - if (nvmem->reg_read) - return nvmem->reg_read(nvmem->priv, offset, val, bytes); + int rc; + size_t new_bytes; + + /* Stop the user from reading */ + if ((offset >= nvmem->size) || (bytes == 0)) + return 0; + + if ((nvmem->reg_read == NULL) || (bytes < nvmem->word_size)) + return -EINVAL; + + if (unlikely(check_add_overflow(bytes, offset, &new_bytes))) + return -EOVERFLOW; - return -EINVAL; + if (new_bytes > nvmem->size) + bytes = nvmem->size - offset; + + bytes = round_down(bytes, nvmem->word_size); + rc = nvmem->reg_read(nvmem->priv, offset, val, bytes); + + if (rc) + return rc; + + return bytes; } static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset, void *val, size_t bytes) { - if (nvmem->reg_write) - return nvmem->reg_write(nvmem->priv, offset, val, bytes); + int rc; + size_t new_bytes; + + /* Stop the user from writing */ + if (offset >= nvmem->size) + return -EFBIG; + + if (bytes == 0) + return 0; + + if ((nvmem->reg_write == NULL) || (bytes < nvmem->word_size)) + return -EINVAL; + + if (unlikely(check_add_overflow(bytes, offset, &new_bytes))) + return -EOVERFLOW; + + if (new_bytes > nvmem->size) + bytes = nvmem->size - offset; + + bytes = round_down(bytes, nvmem->word_size); + rc = nvmem->reg_write(nvmem->priv, offset, val, bytes); + + if (rc) + return rc; - return -EINVAL; + return bytes; } static ssize_t type_show(struct device *dev, @@ -111,33 +152,13 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj, char *buf, loff_t pos, size_t count) { struct device *dev; - struct nvmem_device *nvmem; - int rc; if (attr->private) dev = attr->private; else dev = container_of(kobj, struct device, kobj); - nvmem = to_nvmem_device(dev); - - /* Stop the user from reading */ - if (pos >= nvmem->size) - return 0; - - if (count < nvmem->word_size) - return -EINVAL; - - if (pos + count > nvmem->size) - count = nvmem->size - pos; - - count = round_down(count, nvmem->word_size); - - rc = nvmem_reg_read(nvmem, pos, buf, count); - - if (rc) - return rc; - return count; + return nvmem_reg_read(to_nvmem_device(dev), pos, buf, count); } static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj, @@ -145,33 +166,13 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj, char *buf, loff_t pos, size_t count) { struct device *dev; - struct nvmem_device *nvmem; - int rc; if (attr->private) dev = attr->private; else dev = container_of(kobj, struct device, kobj); - nvmem = to_nvmem_device(dev); - - /* Stop the user from writing */ - if (pos >= nvmem->size) - return -EFBIG; - - if (count < nvmem->word_size) - return -EINVAL; - - if (pos + count > nvmem->size) - count = nvmem->size - pos; - - count = round_down(count, nvmem->word_size); - - rc = nvmem_reg_write(nvmem, pos, buf, count); - - if (rc) - return rc; - return count; + return nvmem_reg_write(to_nvmem_device(dev), pos, buf, count); } /* default read/write permissions */
Add check in nvmem_{read,write}()to ensure that nvmem core never passes an invalid number of bytes. Signed-off-by: Biju Das <biju.das@bp.renesas.com> --- V1-->V2 * Moved checks from bin_attr_nvmem_{read/write} into nvmem_reg_{read,write} * Removed checks from nvmem_device_device_{read,write}() V2-->V3 * Changed the Error from "ENOSPC" to "EFBIG" on nvmem_reg_write. --- drivers/nvmem/core.c | 97 ++++++++++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 48 deletions(-)