Linux-RTC Archive on lore.kernel.org
 help / Atom feed
* [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
@ 2018-12-11  9:24 Biju Das
  2018-12-11  9:55 ` Srinivas Kandagatla
  0 siblings, 1 reply; 6+ messages in thread
From: Biju Das @ 2018-12-11  9:24 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Biju Das, Simon Horman, Geert Uytterhoeven, Chris Paterson,
	Alexandre Belloni, Alessandro Zummo, Fabrizio Castro, linux-rtc,
	linux-renesas-soc

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(-)

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 */
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
  2018-12-11  9:24 [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}() Biju Das
@ 2018-12-11  9:55 ` Srinivas Kandagatla
  2018-12-11 10:54   ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Srinivas Kandagatla @ 2018-12-11  9:55 UTC (permalink / raw)
  To: Biju Das
  Cc: Simon Horman, Geert Uytterhoeven, Chris Paterson,
	Alexandre Belloni, Alessandro Zummo, Fabrizio Castro, linux-rtc,
	linux-renesas-soc



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
  2018-12-11  9:55 ` Srinivas Kandagatla
@ 2018-12-11 10:54   ` Biju Das
  2018-12-11 11:06     ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Biju Das @ 2018-12-11 10:54 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Simon Horman, Geert Uytterhoeven, Chris Paterson,
	Alexandre Belloni, Alessandro Zummo, Fabrizio Castro, linux-rtc,
	linux-renesas-soc

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
  2018-12-11 10:54   ` Biju Das
@ 2018-12-11 11:06     ` Geert Uytterhoeven
  2018-12-11 11:10       ` Srinivas Kandagatla
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2018-12-11 11:06 UTC (permalink / raw)
  To: Biju Das
  Cc: Srinivas Kandagatla, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Alexandre Belloni, Alessandro Zummo,
	Fabrizio Castro, linux-rtc, Linux-Renesas

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
  2018-12-11 11:06     ` Geert Uytterhoeven
@ 2018-12-11 11:10       ` Srinivas Kandagatla
  2018-12-11 11:27         ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Srinivas Kandagatla @ 2018-12-11 11:10 UTC (permalink / raw)
  To: Geert Uytterhoeven, Biju Das
  Cc: Simon Horman, Geert Uytterhoeven, Chris Paterson,
	Alexandre Belloni, Alessandro Zummo, Fabrizio Castro, linux-rtc,
	Linux-Renesas



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").

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}()
  2018-12-11 11:10       ` Srinivas Kandagatla
@ 2018-12-11 11:27         ` Geert Uytterhoeven
  0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2018-12-11 11:27 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Biju Das, Simon Horman, Geert Uytterhoeven, Chris Paterson,
	Alexandre Belloni, Alessandro Zummo, Fabrizio Castro, linux-rtc,
	Linux-Renesas

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11  9:24 [PATCH v3] nvmem: check invalid number of bytes in nvmem_{read,write}() Biju Das
2018-12-11  9:55 ` Srinivas Kandagatla
2018-12-11 10:54   ` Biju Das
2018-12-11 11:06     ` Geert Uytterhoeven
2018-12-11 11:10       ` Srinivas Kandagatla
2018-12-11 11:27         ` Geert Uytterhoeven

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org linux-rtc@archiver.kernel.org
	public-inbox-index linux-rtc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox