* [PATCH] addrmap: Fix off by one in addrmap_set_entry()
@ 2023-07-25 6:50 Dan Carpenter
2023-07-27 0:49 ` Simon Glass
2023-10-30 21:36 ` Tom Rini
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-07-25 6:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: u-boot
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@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;
address_map[idx].vaddr = vaddr;
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
2023-07-25 6:50 [PATCH] addrmap: Fix off by one in addrmap_set_entry() Dan Carpenter
@ 2023-07-27 0:49 ` Simon Glass
2023-07-27 10:56 ` Dan Carpenter
2023-10-30 21:36 ` Tom Rini
1 sibling, 1 reply; 6+ messages in thread
From: Simon Glass @ 2023-07-27 0:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: u-boot
Hi Dan,
On Tue, 25 Jul 2023 at 09:40, Dan Carpenter <dan.carpenter@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@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.
>
> address_map[idx].vaddr = vaddr;
> --
> 2.39.2
>
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
2023-07-27 0:49 ` Simon Glass
@ 2023-07-27 10:56 ` Dan Carpenter
2023-07-28 1:51 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-07-27 10:56 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot
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@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@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()?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
2023-07-27 10:56 ` Dan Carpenter
@ 2023-07-28 1:51 ` Simon Glass
2023-09-12 14:41 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2023-07-28 1:51 UTC (permalink / raw)
To: Dan Carpenter, Bin Meng, Kumar Gala; +Cc: u-boot
Hi Dan,
On Thu, 27 Jul 2023 at 04:56, Dan Carpenter <dan.carpenter@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@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@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
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
2023-07-28 1:51 ` Simon Glass
@ 2023-09-12 14:41 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2023-09-12 14:41 UTC (permalink / raw)
To: Dan Carpenter, Bin Meng, Kumar Gala; +Cc: u-boot
On Thu, 27 Jul 2023 at 19:51, Simon Glass <sjg@google.com> wrote:
>
> Hi Dan,
>
> On Thu, 27 Jul 2023 at 04:56, Dan Carpenter <dan.carpenter@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@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@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@chromium.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
2023-07-25 6:50 [PATCH] addrmap: Fix off by one in addrmap_set_entry() Dan Carpenter
2023-07-27 0:49 ` Simon Glass
@ 2023-10-30 21:36 ` Tom Rini
1 sibling, 0 replies; 6+ messages in thread
From: Tom Rini @ 2023-10-30 21:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: u-boot
[-- Attachment #1: Type: text/plain, Size: 432 bytes --]
On Tue, Jul 25, 2023 at 09:50:40AM +0300, Dan Carpenter 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@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
While Simon had comments of a more general nature that would be good to
handle, start by taking this fix. Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-30 21:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-25 6:50 [PATCH] addrmap: Fix off by one in addrmap_set_entry() Dan Carpenter
2023-07-27 0:49 ` Simon Glass
2023-07-27 10:56 ` Dan Carpenter
2023-07-28 1:51 ` Simon Glass
2023-09-12 14:41 ` Simon Glass
2023-10-30 21:36 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox