Received-SPF: pass (google.com: domain of linux-kernel+bounces-56644-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240206042408.224138-1-joychakr@google.com> <1ecb3744-7baf-4bdd-a01c-8c87fa0a42b3@linaro.org>
 <CAOSNQF2TpA1QXKQBEZXsjXojGcfRKZDjCtLhRUGwLPVfhNWmgA@mail.gmail.com> <feff8e48-c974-447a-99bb-21d5beda1dd1@linaro.org>
In-Reply-To: <feff8e48-c974-447a-99bb-21d5beda1dd1@linaro.org>
From: Joy Chakraborty <joychakr@google.com>
Date: Wed, 7 Feb 2024 20:16:37 +0530
Message-ID: <CAOSNQF0akup9owP6EC0Q7eSrfdsr-sr1bhArF+YQAiAxfRJaVQ@mail.gmail.com>
Subject: Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rob Herring <robh@kernel.org>, 
	Nicolas Saenz Julienne <nsaenz@kernel.org>, linux-kernel@vger.kernel.org, manugautam@google.com, 
	stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 7, 2024 at 3:00=E2=80=AFPM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 07/02/2024 06:35, Joy Chakraborty wrote:
> > On Wed, Feb 7, 2024 at 4:06=E2=80=AFAM 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 err=
or
> >>> as a return value but rmem_read() returns the number of bytes read, a=
s 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 by=
tes
> >>> 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 o=
ffset,
> >>>
> >>>        memunmap(addr);
> >>>
> >>> -     return count;
> >>> +     if (count !=3D bytes) {
> >>
> >> How can this fail unless the values set in priv->mem->size is incorrec=
t
> >>
> >
> > 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 actua=
l
> >> 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 =3D <1>;
> >      #size-cells =3D <1>;
> >      ranges;
> >      nvmem@1000 {
> >          compatible =3D "nvmem-rmem";
> >          reg =3D <0x1000 0x400>;
> >          no-map;
> >          nvmem-layout {
> >              compatible =3D "fixed-layout";
> >              #address-cells =3D <1>;
> >              #size-cells =3D <1>;
> >              calibration@13ff {
> >                  reg =3D <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