[PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

reg_read() callback registered with nvmem core expects an integer error
as a return value but rmem_read() returns the number of bytes read, as a
result error checks in nvmem core fail even when they shouldn't.

Return 0 on success where number of bytes read match the number of bytes
requested and a negative error -EINVAL on all other cases.

Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
Cc: stable@vger.kernel.org
Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/nvmem/rmem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
index 752d0bf4445e..a74dfa279ff4 100644
--- a/drivers/nvmem/rmem.c
+++ b/drivers/nvmem/rmem.c
@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
 
 	memunmap(addr);
 
-	return count;
+	if (count != bytes) {
+		dev_err(priv->dev, "Failed read memory (%d)\n", count);
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int rmem_probe(struct platform_device *pdev)
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Greg Kroah-Hartman]

On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
> reg_read() callback registered with nvmem core expects an integer error
> as a return value but rmem_read() returns the number of bytes read, as a
> result error checks in nvmem core fail even when they shouldn't.
> 
> Return 0 on success where number of bytes read match the number of bytes
> requested and a negative error -EINVAL on all other cases.
> 
> Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  drivers/nvmem/rmem.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> index 752d0bf4445e..a74dfa279ff4 100644
> --- a/drivers/nvmem/rmem.c
> +++ b/drivers/nvmem/rmem.c
> @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
>  
>  	memunmap(addr);
>  
> -	return count;
> +	if (count != bytes) {
> +		dev_err(priv->dev, "Failed read memory (%d)\n", count);
> +		return -EINVAL;

Why is a "short read" somehow illegal here?  What internal changes need
to be made now that this has changed?

And what will userspace do with this error message in the kernel log?

thanks,

greg k-h

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
> > reg_read() callback registered with nvmem core expects an integer error
> > as a return value but rmem_read() returns the number of bytes read, as a
> > result error checks in nvmem core fail even when they shouldn't.
> >
> > Return 0 on success where number of bytes read match the number of bytes
> > requested and a negative error -EINVAL on all other cases.
> >
> > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  drivers/nvmem/rmem.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > index 752d0bf4445e..a74dfa279ff4 100644
> > --- a/drivers/nvmem/rmem.c
> > +++ b/drivers/nvmem/rmem.c
> > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> >
> >       memunmap(addr);
> >
> > -     return count;
> > +     if (count != bytes) {
> > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > +             return -EINVAL;
>
> Why is a "short read" somehow illegal here?  What internal changes need
> to be made now that this has changed?

In my opinion "short read" should be illegal for cases where if the
nvmem core is unable to read the required size of data to fill up a
nvmem cell then data returned might have truncated value.

No internal changes should be made since the registered reg_read() is
called from  __nvmem_reg_read() which eventually passes on the error
code to nvmem_reg_read() whose return code is already checked and
passed to nvmem consumers.
Currently rmem driver is incorrectly passing a positive value for success.

>
> And what will userspace do with this error message in the kernel log?

User space currently is not seeing this error for nvmem device/eeprom
reads due to the following code at nvmem/core.c in
bin_attr_nvmem_read():
"
    rc = nvmem_reg_read(nvmem, pos, buf, count);

    if (rc)
        return rc;

    return count;
"
since it expects to return the number of bytes.

Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read() in current code, which should be fixed on
returning 0 for success.

> thanks,
>
> greg k-h

Thanks
Joy

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Greg Kroah-Hartman]

On Tue, Feb 06, 2024 at 04:01:02PM +0530, Joy Chakraborty wrote:
> On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
> > > reg_read() callback registered with nvmem core expects an integer error
> > > as a return value but rmem_read() returns the number of bytes read, as a
> > > result error checks in nvmem core fail even when they shouldn't.
> > >
> > > Return 0 on success where number of bytes read match the number of bytes
> > > requested and a negative error -EINVAL on all other cases.
> > >
> > > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > > ---
> > >  drivers/nvmem/rmem.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > > index 752d0bf4445e..a74dfa279ff4 100644
> > > --- a/drivers/nvmem/rmem.c
> > > +++ b/drivers/nvmem/rmem.c
> > > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> > >
> > >       memunmap(addr);
> > >
> > > -     return count;
> > > +     if (count != bytes) {
> > > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > > +             return -EINVAL;
> >
> > Why is a "short read" somehow illegal here?  What internal changes need
> > to be made now that this has changed?
> 
> In my opinion "short read" should be illegal for cases where if the
> nvmem core is unable to read the required size of data to fill up a
> nvmem cell then data returned might have truncated value.

But that's kind of against what a read() call normally expects.

> No internal changes should be made since the registered reg_read() is
> called from  __nvmem_reg_read() which eventually passes on the error
> code to nvmem_reg_read() whose return code is already checked and
> passed to nvmem consumers.
> Currently rmem driver is incorrectly passing a positive value for success.

So this is an internal api issue and not a general issue?  Unwinding the
read callbacks here is hard.

Also, in looking at the code, how can this ever be a short read?  You
are using memory_read_from_buffer() which unless the values passed into
it are incorrect, will always return the expected read amount.

> > And what will userspace do with this error message in the kernel log?
> 
> User space currently is not seeing this error for nvmem device/eeprom
> reads due to the following code at nvmem/core.c in
> bin_attr_nvmem_read():
> "
>     rc = nvmem_reg_read(nvmem, pos, buf, count);
> 
>     if (rc)
>         return rc;
> 
>     return count;
> "
> since it expects to return the number of bytes.
> 
> Userspace will see a false error with nvmem cell reads from
> nvmem_cell_attr_read() in current code, which should be fixed on
> returning 0 for success.

So maybe fix this all up to allow the read to return the actual amount
read?  That feels more "correct" to me.

thanks,

greg k-h

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Tue, Feb 6, 2024 at 4:27 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 06, 2024 at 04:01:02PM +0530, Joy Chakraborty wrote:
> > On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
> > > > reg_read() callback registered with nvmem core expects an integer error
> > > > as a return value but rmem_read() returns the number of bytes read, as a
> > > > result error checks in nvmem core fail even when they shouldn't.
> > > >
> > > > Return 0 on success where number of bytes read match the number of bytes
> > > > requested and a negative error -EINVAL on all other cases.
> > > >
> > > > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > > > ---
> > > >  drivers/nvmem/rmem.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > > > index 752d0bf4445e..a74dfa279ff4 100644
> > > > --- a/drivers/nvmem/rmem.c
> > > > +++ b/drivers/nvmem/rmem.c
> > > > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> > > >
> > > >       memunmap(addr);
> > > >
> > > > -     return count;
> > > > +     if (count != bytes) {
> > > > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > > > +             return -EINVAL;
> > >
> > > Why is a "short read" somehow illegal here?  What internal changes need
> > > to be made now that this has changed?
> >
> > In my opinion "short read" should be illegal for cases where if the
> > nvmem core is unable to read the required size of data to fill up a
> > nvmem cell then data returned might have truncated value.
>
> But that's kind of against what a read() call normally expects.

That is fair, maybe the size check should be there at the nvmem core
layer to catch any truncations but the actual size read is not passed
from provider to the core layer.

>
> > No internal changes should be made since the registered reg_read() is
> > called from  __nvmem_reg_read() which eventually passes on the error
> > code to nvmem_reg_read() whose return code is already checked and
> > passed to nvmem consumers.
> > Currently rmem driver is incorrectly passing a positive value for success.
>
> So this is an internal api issue and not a general issue?  Unwinding the
> read callbacks here is hard.

Yes, this is an internal API issue with how the return type of the
reg_read() function pointer passed to nvmem_register() is handled.
The function prototype in nvmem-provider.h does not define how the
return value is treated in nvmem core.
"
typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
void *val, size_t bytes);
"
Currently it is always checked against 0 for success in nvmem/core.c
which all nvmem-providers adhere to i.e. return 0 on success.
Actual size read from the provider is considered to be equal to the
requested size from core as the provider does not relay that
information.

>
> Also, in looking at the code, how can this ever be a short read?  You
> are using memory_read_from_buffer() which unless the values passed into
> it are incorrect, will always return the expected read amount.
>

Correct, we will have an error only if the returned value from
memory_read_from_buffer() is negative.
So to work with the current nvmem core implementation, I can return
the value as is if negative and 0 for positive.

> > > And what will userspace do with this error message in the kernel log?
> >
> > User space currently is not seeing this error for nvmem device/eeprom
> > reads due to the following code at nvmem/core.c in
> > bin_attr_nvmem_read():
> > "
> >     rc = nvmem_reg_read(nvmem, pos, buf, count);
> >
> >     if (rc)
> >         return rc;
> >
> >     return count;
> > "
> > since it expects to return the number of bytes.
> >
> > Userspace will see a false error with nvmem cell reads from
> > nvmem_cell_attr_read() in current code, which should be fixed on
> > returning 0 for success.
>
> So maybe fix this all up to allow the read to return the actual amount
> read?  That feels more "correct" to me.
>

If I change the behavior of the nvmem_reg_read_t callback to negative
for error and number of bytes actually read for success then, other
than the core driver I would also have to change all the
nvmem-provider drivers.
Is it okay to do so ?

> thanks,
>
> greg k-h

Thanks
Joy

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Greg Kroah-Hartman]

On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> > > Userspace will see a false error with nvmem cell reads from
> > > nvmem_cell_attr_read() in current code, which should be fixed on
> > > returning 0 for success.
> >
> > So maybe fix this all up to allow the read to return the actual amount
> > read?  That feels more "correct" to me.
> >
> 
> If I change the behavior of the nvmem_reg_read_t callback to negative
> for error and number of bytes actually read for success then, other
> than the core driver I would also have to change all the
> nvmem-provider drivers.
> Is it okay to do so ?

Sure, why not?  That seems like the correct fix to me, right?

thanks,

greg k-h

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> > > > Userspace will see a false error with nvmem cell reads from
> > > > nvmem_cell_attr_read() in current code, which should be fixed on
> > > > returning 0 for success.
> > >
> > > So maybe fix this all up to allow the read to return the actual amount
> > > read?  That feels more "correct" to me.
> > >
> >
> > If I change the behavior of the nvmem_reg_read_t callback to negative
> > for error and number of bytes actually read for success then, other
> > than the core driver I would also have to change all the
> > nvmem-provider drivers.
> > Is it okay to do so ?
>
> Sure, why not?  That seems like the correct fix to me, right?

Sure, I can do that.

Is it okay to change the if checks on the return code to "if (rc < 0)"
instead of "if (rc)" as a fix for the immediate issue with how return
value from rmem is handled which can be applied to older kernels.
In a separate patch I can change the definition of nvmem_reg_read_t()
to return ssize_t instead of int and make corresponding changes to
nvmem-provider drivers.

Does that sound okay ?
>
> thanks,
>
> greg k-h

Thanks
Joy

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty <joychakr@google.com> wrote:
>
> On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> > > > > Userspace will see a false error with nvmem cell reads from
> > > > > nvmem_cell_attr_read() in current code, which should be fixed on
> > > > > returning 0 for success.
> > > >
> > > > So maybe fix this all up to allow the read to return the actual amount
> > > > read?  That feels more "correct" to me.
> > > >
> > >
> > > If I change the behavior of the nvmem_reg_read_t callback to negative
> > > for error and number of bytes actually read for success then, other
> > > than the core driver I would also have to change all the
> > > nvmem-provider drivers.
> > > Is it okay to do so ?
> >
> > Sure, why not?  That seems like the correct fix to me, right?
>
> Sure, I can do that.
>
> Is it okay to change the if checks on the return code to "if (rc < 0)"
> instead of "if (rc)" as a fix for the immediate issue with how return
> value from rmem is handled which can be applied to older kernels.
> In a separate patch I can change the definition of nvmem_reg_read_t()
> to return ssize_t instead of int and make corresponding changes to
> nvmem-provider drivers.
>
> Does that sound okay ?

Hi Greg,

Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/
to change the return type for read/write callbacks.
Do I mark that with the "Fixes:" tag as well ?
It affects a lot of files so might not be able to easily pick to an
older kernel when needed.

Thanks
Joy

> >
> > thanks,
> >
> > greg k-h
>
> Thanks
> Joy

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Greg Kroah-Hartman]

On Tue, Feb 27, 2024 at 12:27:09PM +0530, Joy Chakraborty wrote:
> On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty <joychakr@google.com> wrote:
> >
> > On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> > > > > > Userspace will see a false error with nvmem cell reads from
> > > > > > nvmem_cell_attr_read() in current code, which should be fixed on
> > > > > > returning 0 for success.
> > > > >
> > > > > So maybe fix this all up to allow the read to return the actual amount
> > > > > read?  That feels more "correct" to me.
> > > > >
> > > >
> > > > If I change the behavior of the nvmem_reg_read_t callback to negative
> > > > for error and number of bytes actually read for success then, other
> > > > than the core driver I would also have to change all the
> > > > nvmem-provider drivers.
> > > > Is it okay to do so ?
> > >
> > > Sure, why not?  That seems like the correct fix to me, right?
> >
> > Sure, I can do that.
> >
> > Is it okay to change the if checks on the return code to "if (rc < 0)"
> > instead of "if (rc)" as a fix for the immediate issue with how return
> > value from rmem is handled which can be applied to older kernels.
> > In a separate patch I can change the definition of nvmem_reg_read_t()
> > to return ssize_t instead of int and make corresponding changes to
> > nvmem-provider drivers.
> >
> > Does that sound okay ?
> 
> Hi Greg,
> 
> Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/
> to change the return type for read/write callbacks.
> Do I mark that with the "Fixes:" tag as well ?

Is it actually fixing a bug?  Or just evolving the api to be more
correct over time?  I think the latter.

> It affects a lot of files so might not be able to easily pick to an
> older kernel when needed.

What is it fixing that older kernels need?

And do not worry about stable kernels while doing development, only
take that into consideration after your changes are accepted.

thanks,

greg k-h

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Tue, Feb 27, 2024 at 1:02 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 27, 2024 at 12:27:09PM +0530, Joy Chakraborty wrote:
> > On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty <joychakr@google.com> wrote:
> > >
> > > On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> > > > > > > Userspace will see a false error with nvmem cell reads from
> > > > > > > nvmem_cell_attr_read() in current code, which should be fixed on
> > > > > > > returning 0 for success.
> > > > > >
> > > > > > So maybe fix this all up to allow the read to return the actual amount
> > > > > > read?  That feels more "correct" to me.
> > > > > >
> > > > >
> > > > > If I change the behavior of the nvmem_reg_read_t callback to negative
> > > > > for error and number of bytes actually read for success then, other
> > > > > than the core driver I would also have to change all the
> > > > > nvmem-provider drivers.
> > > > > Is it okay to do so ?
> > > >
> > > > Sure, why not?  That seems like the correct fix to me, right?
> > >
> > > Sure, I can do that.
> > >
> > > Is it okay to change the if checks on the return code to "if (rc < 0)"
> > > instead of "if (rc)" as a fix for the immediate issue with how return
> > > value from rmem is handled which can be applied to older kernels.
> > > In a separate patch I can change the definition of nvmem_reg_read_t()
> > > to return ssize_t instead of int and make corresponding changes to
> > > nvmem-provider drivers.
> > >
> > > Does that sound okay ?
> >
> > Hi Greg,
> >
> > Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/
> > to change the return type for read/write callbacks.
> > Do I mark that with the "Fixes:" tag as well ?
>
> Is it actually fixing a bug?  Or just evolving the api to be more
> correct over time?  I think the latter.

It is more of the latter i.e. evolving the API to be correct but
indirectly it ends up fixing this bug where positive value returned by
this rmem driver is treated as an error in nvmem core.

>
> > It affects a lot of files so might not be able to easily pick to an
> > older kernel when needed.
>
> What is it fixing that older kernels need?

It is fixing the handling of a positive value returned by this rmem
driver which will be treated as an error in older kernels as the nvmem
core expects 0 on success without changing API behavior.

>
> And do not worry about stable kernels while doing development, only
> take that into consideration after your changes are accepted.

Got it, thank you for your input .

>
> thanks,
>
> greg k-h

Thanks
Joy

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Srinivas Kandagatla]

On 06/02/2024 04:24, Joy Chakraborty wrote:
> reg_read() callback registered with nvmem core expects an integer error
> as a return value but rmem_read() returns the number of bytes read, as a
> result error checks in nvmem core fail even when they shouldn't.
> 
> Return 0 on success where number of bytes read match the number of bytes
> requested and a negative error -EINVAL on all other cases.
> 
> Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>   drivers/nvmem/rmem.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> index 752d0bf4445e..a74dfa279ff4 100644
> --- a/drivers/nvmem/rmem.c
> +++ b/drivers/nvmem/rmem.c
> @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
>   
>   	memunmap(addr);
>   
> -	return count;
> +	if (count != bytes) {

How can this fail unless the values set in priv->mem->size is incorrect

Only case I see this failing with short reads is when offset cross the 
boundary of priv->mem->size.


can you provide more details on the failure usecase, may be with actual 
values of offsets, bytes and priv->mem->size?


> +		dev_err(priv->dev, "Failed read memory (%d)\n", count);
> +		return -EINVAL;
> +	}
> +

> +	return 0;

thanks,
srini

>   }
>   
>   static int rmem_probe(struct platform_device *pdev)

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 06/02/2024 04:24, Joy Chakraborty wrote:
> > reg_read() callback registered with nvmem core expects an integer error
> > as a return value but rmem_read() returns the number of bytes read, as a
> > result error checks in nvmem core fail even when they shouldn't.
> >
> > Return 0 on success where number of bytes read match the number of bytes
> > requested and a negative error -EINVAL on all other cases.
> >
> > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >   drivers/nvmem/rmem.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > index 752d0bf4445e..a74dfa279ff4 100644
> > --- a/drivers/nvmem/rmem.c
> > +++ b/drivers/nvmem/rmem.c
> > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> >
> >       memunmap(addr);
> >
> > -     return count;
> > +     if (count != bytes) {
>
> How can this fail unless the values set in priv->mem->size is incorrect
>

That should be correct since it would be fetched from the reserved
memory definition in the device tree.

> Only case I see this failing with short reads is when offset cross the
> boundary of priv->mem->size.
>
>
> can you provide more details on the failure usecase, may be with actual
> values of offsets, bytes and priv->mem->size?
>

This could very well happen if a fixed-layout defined for the reserved
memory has a cell which defines an offset and size greater than the
actual size of the reserved mem.
For E.g. if the device tree node is as follows
reserved-memory {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;
    nvmem@1000 {
        compatible = "nvmem-rmem";
        reg = <0x1000 0x400>;
        no-map;
        nvmem-layout {
            compatible = "fixed-layout";
            #address-cells = <1>;
            #size-cells = <1>;
            calibration@13ff {
                reg = <0x13ff 0x2>;
            };
        };
    };
};
If we try to read the cell "calibration" which crosses the boundary of
the reserved memory then it will lead to a short read.
Though, one might argue that the protection against such cell
definition should be there during fixed-layout parsing in core itself
but that is not there now and would not be a fix.

What I am trying to fix here is not exactly short reads but how the
return value of rmem_read() is treated by the nvmem core, where it
treats a non-zero return from read as an error currently. Hence
returning the number of bytes read leads to false failures if we try
to read a cell.


>
> > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > +             return -EINVAL;
> > +     }
> > +
>
> > +     return 0;
>
> thanks,
> srini
>
> >   }
> >
> >   static int rmem_probe(struct platform_device *pdev)

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Srinivas Kandagatla]

On 07/02/2024 06:35, Joy Chakraborty wrote:
> On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>
>> On 06/02/2024 04:24, Joy Chakraborty wrote:
>>> reg_read() callback registered with nvmem core expects an integer error
>>> as a return value but rmem_read() returns the number of bytes read, as a
>>> result error checks in nvmem core fail even when they shouldn't.
>>>
>>> Return 0 on success where number of bytes read match the number of bytes
>>> requested and a negative error -EINVAL on all other cases.
>>>
>>> Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
>>> ---
>>>    drivers/nvmem/rmem.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
>>> index 752d0bf4445e..a74dfa279ff4 100644
>>> --- a/drivers/nvmem/rmem.c
>>> +++ b/drivers/nvmem/rmem.c
>>> @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
>>>
>>>        memunmap(addr);
>>>
>>> -     return count;
>>> +     if (count != bytes) {
>>
>> How can this fail unless the values set in priv->mem->size is incorrect
>>
> 
> That should be correct since it would be fetched from the reserved
> memory definition in the device tree.
> 
>> Only case I see this failing with short reads is when offset cross the
>> boundary of priv->mem->size.
>>
>>
>> can you provide more details on the failure usecase, may be with actual
>> values of offsets, bytes and priv->mem->size?
>>
> 
> This could very well happen if a fixed-layout defined for the reserved
> memory has a cell which defines an offset and size greater than the
> actual size of the reserved mem.

No that should just be blocked from core layer, atleast which is what is 
checked bin_attr_nvmem_read(), if checks are missing in other places 
then that needs fixing.


> For E.g. if the device tree node is as follows
> reserved-memory {
>      #address-cells = <1>;
>      #size-cells = <1>;
>      ranges;
>      nvmem@1000 {
>          compatible = "nvmem-rmem";
>          reg = <0x1000 0x400>;
>          no-map;
>          nvmem-layout {
>              compatible = "fixed-layout";
>              #address-cells = <1>;
>              #size-cells = <1>;
>              calibration@13ff {
>                  reg = <0x13ff 0x2>;

this is out of range, core should just err out.

--srini

>              };
>          };
>      };
> };
> If we try to read the cell "calibration" which crosses the boundary of
> the reserved memory then it will lead to a short read.
> Though, one might argue that the protection against such cell
> definition should be there during fixed-layout parsing in core itself
> but that is not there now and would not be a fix.
> 
> What I am trying to fix here is not exactly short reads but how the
> return value of rmem_read() is treated by the nvmem core, where it
> treats a non-zero return from read as an error currently. Hence
> returning the number of bytes read leads to false failures if we try
> to read a cell.
> 
> 
>>
>>> +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>
>>> +     return 0;
>>
>> thanks,
>> srini
>>
>>>    }
>>>
>>>    static int rmem_probe(struct platform_device *pdev)

Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Wed, Feb 7, 2024 at 3:00 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 07/02/2024 06:35, Joy Chakraborty wrote:
> > On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >>
> >>
> >> On 06/02/2024 04:24, Joy Chakraborty wrote:
> >>> reg_read() callback registered with nvmem core expects an integer error
> >>> as a return value but rmem_read() returns the number of bytes read, as a
> >>> result error checks in nvmem core fail even when they shouldn't.
> >>>
> >>> Return 0 on success where number of bytes read match the number of bytes
> >>> requested and a negative error -EINVAL on all other cases.
> >>>
> >>> Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> >>> ---
> >>>    drivers/nvmem/rmem.c | 7 ++++++-
> >>>    1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> >>> index 752d0bf4445e..a74dfa279ff4 100644
> >>> --- a/drivers/nvmem/rmem.c
> >>> +++ b/drivers/nvmem/rmem.c
> >>> @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> >>>
> >>>        memunmap(addr);
> >>>
> >>> -     return count;
> >>> +     if (count != bytes) {
> >>
> >> How can this fail unless the values set in priv->mem->size is incorrect
> >>
> >
> > That should be correct since it would be fetched from the reserved
> > memory definition in the device tree.
> >
> >> Only case I see this failing with short reads is when offset cross the
> >> boundary of priv->mem->size.
> >>
> >>
> >> can you provide more details on the failure usecase, may be with actual
> >> values of offsets, bytes and priv->mem->size?
> >>
> >
> > This could very well happen if a fixed-layout defined for the reserved
> > memory has a cell which defines an offset and size greater than the
> > actual size of the reserved mem.
>
> No that should just be blocked from core layer, atleast which is what is
> checked bin_attr_nvmem_read(), if checks are missing in other places
> then that needs fixing.
>

Sure.

>
> > For E.g. if the device tree node is as follows
> > reserved-memory {
> >      #address-cells = <1>;
> >      #size-cells = <1>;
> >      ranges;
> >      nvmem@1000 {
> >          compatible = "nvmem-rmem";
> >          reg = <0x1000 0x400>;
> >          no-map;
> >          nvmem-layout {
> >              compatible = "fixed-layout";
> >              #address-cells = <1>;
> >              #size-cells = <1>;
> >              calibration@13ff {
> >                  reg = <0x13ff 0x2>;
>
> this is out of range, core should just err out.
>

Cells are currently unchecked, I can fix that in a different patch.

> --srini
>
> >              };
> >          };
> >      };
> > };
> > If we try to read the cell "calibration" which crosses the boundary of
> > the reserved memory then it will lead to a short read.
> > Though, one might argue that the protection against such cell
> > definition should be there during fixed-layout parsing in core itself
> > but that is not there now and would not be a fix.
> >
> > What I am trying to fix here is not exactly short reads but how the
> > return value of rmem_read() is treated by the nvmem core, where it
> > treats a non-zero return from read as an error currently. Hence
> > returning the number of bytes read leads to false failures if we try
> > to read a cell.
> >
> >
> >>
> >>> +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>
> >>> +     return 0;
> >>
> >> thanks,
> >> srini
> >>
> >>>    }
> >>>
> >>>    static int rmem_probe(struct platform_device *pdev)

Thanks
Joy