[PATCH] addrmap: Fix off by one in addrmap_set_entry()

[Simon Glass]

On Thu, 27 Jul 2023 at 19:51, Simon Glass <sjg at google.com> wrote:
>
> Hi Dan,
>
> On Thu, 27 Jul 2023 at 04:56, Dan Carpenter <dan.carpenter at linaro.org> wrote:
> >
> > On Wed, Jul 26, 2023 at 06:49:44PM -0600, Simon Glass wrote:
> > > Hi Dan,
> > >
> > > On Tue, 25 Jul 2023 at 09:40, Dan Carpenter <dan.carpenter at linaro.org> wrote:
> > > >
> > > > The > comparison needs to be changed to >= to prevent an out of bounds
> > > > write on th next line.
> > > >
> > > > Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
> > > > ---
> > > >  lib/addr_map.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/addr_map.c b/lib/addr_map.c
> > > > index 9b3e0a544e47..86e932e4b561 100644
> > > > --- a/lib/addr_map.c
> > > > +++ b/lib/addr_map.c
> > > > @@ -59,7 +59,7 @@ void *addrmap_phys_to_virt(phys_addr_t paddr)
> > > >  void addrmap_set_entry(unsigned long vaddr, phys_addr_t paddr,
> > > >                         phys_size_t size, int idx)
> > > >  {
> > > > -       if (idx > CONFIG_SYS_NUM_ADDR_MAP)
> > > > +       if (idx >= CONFIG_SYS_NUM_ADDR_MAP)
> > > >                 return;
> > >
> > > It looks like this function should return an error.
> > >
> >
> > If we hit this error path there probably isn't a reasonable way to
> > recover.  Maybe we could add a WARN()?
>
> When we get an error we should report it. For leaf functions like
> this, WARN adds to code size for a case that should not happen. The
> caller will need to check the error and fail. The function should
> always have had an error-return code, by the look of it. If we adopt
> the approach of warning instead of returning error codes,
>
> +Bin Meng
> +Kumar Gala

Since this is a fix, separate from the poor API:

Reviewed-by: Simon Glass <sjg at chromium.org>