diff mbox series

[v4,1/5] nvmem: check invalid number of bytes in nvmem_device_{read,write}

Message ID 1544182066-31528-2-git-send-email-biju.das@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add NXP pcf85263 real-time clock support | expand

Commit Message

Biju Das Dec. 7, 2018, 11:27 a.m. UTC
Add check in nvmem_device_{read,write}()to ensure that nvmem core never
passes an invalid number of bytes.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V3-->V4
	* New patch.
---
 drivers/nvmem/core.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Srinivas Kandagatla Dec. 10, 2018, 12:37 p.m. UTC | #1
Thanks for the patch.

On 07/12/18 11:27, Biju Das wrote:
> Add check in nvmem_device_{read,write}()to ensure that nvmem core never
> passes an invalid number of bytes.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
> V3-->V4
> 	* New patch.
> ---
>   drivers/nvmem/core.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 

Its better to move checks from 
bin_attr_nvmem_read()/bin_attr_nvmem_write() into nvmem_reg_read() and 
nvmem_reg_write(), so its easy to maintain, rather than adding them to 
each function.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index d9fd110..db7de33 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1433,10 +1433,21 @@ int nvmem_device_read(struct nvmem_device *nvmem,
>   		      size_t bytes, void *buf)
>   {
>   	int rc;
> +	size_t new_bytes;
>   
>   	if (!nvmem)
>   		return -EINVAL;
>   
> +	/* Stop the user from reading */
> +	if ((offset >= nvmem->size) || (bytes == 0))
> +		return 0;
> +
> +	if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
> +		return -EOVERFLOW;
> +
> +	if (new_bytes > nvmem->size)
> +		bytes = nvmem->size - offset;
> +
>   	rc = nvmem_reg_read(nvmem, offset, buf, bytes);
>   
>   	if (rc)
> @@ -1461,16 +1472,29 @@ int nvmem_device_write(struct nvmem_device *nvmem,
>   		       size_t bytes, void *buf)
>   {
>   	int rc;
> +	size_t new_bytes;
>   
>   	if (!nvmem)
>   		return -EINVAL;
>   
> +	/* Stop the user from writing */
> +	if (offset >= nvmem->size)
> +		return -ENOSPC;
> +
> +	if (bytes == 0)
> +		return 0;
> +
> +	if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
> +		return -EOVERFLOW;
> +
> +	if (new_bytes > nvmem->size)
> +		bytes = nvmem->size - offset;
> +
>   	rc = nvmem_reg_write(nvmem, offset, buf, bytes);
>   
>   	if (rc)
>   		return rc;
>   
> -

Unrelated change!


--srini
>   	return bytes;
>   }
>   EXPORT_SYMBOL_GPL(nvmem_device_write);
>
Biju Das Dec. 10, 2018, 12:45 p.m. UTC | #2
Hi Srinivas,

Thanks for the feedback.

> Subject: Re: [PATCH v4 1/5] nvmem: check invalid number of bytes in
> nvmem_device_{read,write}
>
> Thanks for the patch.
>
> On 07/12/18 11:27, Biju Das wrote:
> > Add check in nvmem_device_{read,write}()to ensure that nvmem core
> > never passes an invalid number of bytes.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> > V3-->V4
> > * New patch.
> > ---
> >   drivers/nvmem/core.c | 26 +++++++++++++++++++++++++-
> >   1 file changed, 25 insertions(+), 1 deletion(-)
> >
>
> Its better to move checks from
> bin_attr_nvmem_read()/bin_attr_nvmem_write() into nvmem_reg_read()
> and nvmem_reg_write(), so its easy to maintain, rather than adding them to
> each function.

Initially I also thought the same. But there are some RTC drivers which is using nvmem_device
interface  is not defining "word_size"  member. That is the reason we need to add it in separate function.
Can you please suggest how to handle the above drivers?

> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index
> > d9fd110..db7de33 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -1433,10 +1433,21 @@ int nvmem_device_read(struct nvmem_device
> *nvmem,
> >         size_t bytes, void *buf)
> >   {
> >   int rc;
> > +size_t new_bytes;
> >
> >   if (!nvmem)
> >   return -EINVAL;
> >
> > +/* Stop the user from reading */
> > +if ((offset >= nvmem->size) || (bytes == 0))
> > +return 0;
> > +
> > +if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
> > +return -EOVERFLOW;
> > +
> > +if (new_bytes > nvmem->size)
> > +bytes = nvmem->size - offset;
> > +
> >   rc = nvmem_reg_read(nvmem, offset, buf, bytes);
> >
> >   if (rc)
> > @@ -1461,16 +1472,29 @@ int nvmem_device_write(struct
> nvmem_device *nvmem,
> >          size_t bytes, void *buf)
> >   {
> >   int rc;
> > +size_t new_bytes;
> >
> >   if (!nvmem)
> >   return -EINVAL;
> >
> > +/* Stop the user from writing */
> > +if (offset >= nvmem->size)
> > +return -ENOSPC;
> > +
> > +if (bytes == 0)
> > +return 0;
> > +
> > +if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
> > +return -EOVERFLOW;
> > +
> > +if (new_bytes > nvmem->size)
> > +bytes = nvmem->size - offset;
> > +
> >   rc = nvmem_reg_write(nvmem, offset, buf, bytes);
> >
> >   if (rc)
> >   return rc;
> >
> > -
>
> Unrelated change!

I  found an extra space in this function.  If it is a problem, I will send V2 for undoing this.
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.
Srinivas Kandagatla Dec. 10, 2018, 12:55 p.m. UTC | #3
On 10/12/18 12:45, Biju Das wrote:
>> Its better to move checks from
>> bin_attr_nvmem_read()/bin_attr_nvmem_write() into nvmem_reg_read()
>> and nvmem_reg_write(), so its easy to maintain, rather than adding them to
>> each function.
> Initially I also thought the same. But there are some RTC drivers which is using nvmem_device
> interface  is not defining "word_size"  member. That is the reason we need to add it in separate function.
> Can you please suggest how to handle the above drivers?

Word size is optional anyway, in the case where word_size is not 
specified, its considered as one byte word_size.

--srini

>
Biju Das Dec. 10, 2018, 3:35 p.m. UTC | #4
Hi Srinivas,

Thanks for the feedback.

> Subject: Re: [PATCH v4 1/5] nvmem: check invalid number of bytes in
> nvmem_device_{read,write}
>
>
>
> On 10/12/18 12:45, Biju Das wrote:
> >> Its better to move checks from
> >> bin_attr_nvmem_read()/bin_attr_nvmem_write() into
> nvmem_reg_read()
> >> and nvmem_reg_write(), so its easy to maintain, rather than adding
> >> them to each function.
> > Initially I also thought the same. But there are some RTC drivers
> > which is using nvmem_device interface  is not defining "word_size"
> member. That is the reason we need to add it in separate function.
> > Can you please suggest how to handle the above drivers?
>
> Word size is optional anyway, in the case where word_size is not specified,
> its considered as one byte word_size.

Will send V2 for this.

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.
diff mbox series

Patch

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index d9fd110..db7de33 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1433,10 +1433,21 @@  int nvmem_device_read(struct nvmem_device *nvmem,
 		      size_t bytes, void *buf)
 {
 	int rc;
+	size_t new_bytes;
 
 	if (!nvmem)
 		return -EINVAL;
 
+	/* Stop the user from reading */
+	if ((offset >= nvmem->size) || (bytes == 0))
+		return 0;
+
+	if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
+		return -EOVERFLOW;
+
+	if (new_bytes > nvmem->size)
+		bytes = nvmem->size - offset;
+
 	rc = nvmem_reg_read(nvmem, offset, buf, bytes);
 
 	if (rc)
@@ -1461,16 +1472,29 @@  int nvmem_device_write(struct nvmem_device *nvmem,
 		       size_t bytes, void *buf)
 {
 	int rc;
+	size_t new_bytes;
 
 	if (!nvmem)
 		return -EINVAL;
 
+	/* Stop the user from writing */
+	if (offset >= nvmem->size)
+		return -ENOSPC;
+
+	if (bytes == 0)
+		return 0;
+
+	if (unlikely(check_add_overflow(bytes, offset, &new_bytes)))
+		return -EOVERFLOW;
+
+	if (new_bytes > nvmem->size)
+		bytes = nvmem->size - offset;
+
 	rc = nvmem_reg_write(nvmem, offset, buf, bytes);
 
 	if (rc)
 		return rc;
 
-
 	return bytes;
 }
 EXPORT_SYMBOL_GPL(nvmem_device_write);