Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2015-10-07 at 18:56 +0100, Ard Biesheuvel wrote:
> On 7 October 2015 at 17:02, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On 7 October 2015 at 16:49, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
> > > > This fixes two issues with the EFI libstub code that removes the memory
> > > > nodes from the FDT:
> > > > a) fdt_del_node() invalidates the iterator, so we should restart from the
> > > >    top after calling it;
> > > > b) use fixed length of 6 when matching against 'memory' using strncmp(),
> > > >    since otherwise, substrings like 'm' or 'mem' could match as well.
> > > > 
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >  drivers/firmware/efi/libstub/fdt.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > > > index 0ef16923b4f0..2c7d09936479 100644
> > > > --- a/drivers/firmware/efi/libstub/fdt.c
> > > > +++ b/drivers/firmware/efi/libstub/fdt.c
> > > > @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> > > >        * Delete any memory nodes present. We must delete nodes which
> > > >        * early_init_dt_scan_memory may try to use.
> > > >        */
> > > > +restart:
> > > >       prev = 0;
> > > >       for (;;) {
> > > >               const char *type;
> > > > @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> > > >                       break;
> > > > 
> > > >               type = fdt_getprop(fdt, node, "device_type", &len);
> > > > -             if (type && strncmp(type, "memory", len) == 0) {
> > > > +             if (type && strncmp(type, "memory", 6) == 0) {
> > > 
> > > I see what you're trying to fix here, but I think using 6 is wrong
> > > also. A plain strcmp() would work as long as property is a string.
> > > 
> > 
> > That is what Leif suggested as well, but it pulls in yet another
> > string function into the stub, so I tried to avoid it. I think this
> > particular case is safe, since the property names are in the string
> > table, which is after the nodes in the DTB structure, so 'type' is
> > guaranteed not to point right before the end of a mapped region.
> > 
> 
> ... or is your concern about matching on longer strings that only
> start with 'memory'?

Yes. And the old code would match shorted strings than "memory".

> 
> I guess it makes sense to use strcmp() after all, only arm64 does not
> have an asm implementation, so under the stricter rules that I am
> proposing for fencing off the EFI stub code from the kernel proper, I
> will have to go and add a strcmp() implementation to
> drivers/firmware/efi/libstub/string.c.
> 
> But still better than this, I guess ...

You could also add an explicit length comparison but adding strcmp
seems pretty reasonable.

> 
> 
> > > 
> > > >                       fdt_del_node(fdt, node);
> > > > -                     continue;
> > > > +                     goto restart;
> > > >               }
> > > > 
> > > >               prev = node;
> > > 
> > > Maybe do:
> > > 
> > >                 if (type && strncmp(type, "memory", len) == 0) {
> > >                         fdt_del_node(fdt, node);
> > > -                       continue;
> > > -               }
> > > -
> > > -               prev = node;
> > > +                       prev = NULL;
> > > +               } else
> > > +                       prev = node;
> > >         }
> > > 
> > > 
> > > instead of adding a goto.
> > > 
> > 
> > Yes, that is actually better. Thanks.