Re: [Xen-devel] [PATCH v6 05/26] xen/arm: check for multiboot nodes only under /chosen

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 11/9/18 9:38 PM, Stefano Stabellini wrote:
> On Fri, 9 Nov 2018, Julien Grall wrote:
> > Hi,
> >
> > On 02/11/2018 23:44, Stefano Stabellini wrote:
> > > Make sure to only look for multiboot compatible nodes only under
> > > /chosen, not under any other paths (depth <= 3).
> > >
> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > >
> > > ---
> > >
> > > Changes in v6:
> > > - do not proceed if fdt_get_path returns error != -FDT_ERR_NOSPACE
> > > - remove sizeof, use hardcoded value
> > >
> > > Changes in v5:
> > > - add patch
> > > - add check on return value of fdt_get_path
> > > ---
> > >    xen/arch/arm/bootfdt.c | 14 +++++++++++---
> > >    1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index 8eba42c..a42fe87 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -173,7 +173,15 @@ static void __init process_multiboot_node(const void
> > > *fdt, int node,
> > >        bootmodule_kind kind;
> > >        paddr_t start, size;
> > >        const char *cmdline;
> > > -    int len;
> > > +    int len = 8; /* sizeof "/chosen" */
> > > +    char path[8];
> > > +    int ret;
> > > +
> > > +    /* Check that the node is under "chosen" */
> > > +    ret = fdt_get_path(fdt, node, path, len);
> > > +    if ( (ret != 0 && ret != -FDT_ERR_NOSPACE) ||
> > I don't understand why you specifically check for -FDT_ERR_NOSPACE here.
> >
> > Looking at fdt_get_path(...) there are case where the function may return
> > -FDT_ERR_NOSPACE yet not filling up path. So you would end up to compare
> > garbage.
> I am explicitely checking for -FDT_ERR_NOSPACE because it is a valid
> condition in this case: -FDT_ERR_NOSPACE is returned where `path' is not
> big enough to contain the full device tree path. It is OK and expected,
> given that path is only 8 chars long. So, in case of -FDT_ERR_NOSPACE,
> we should continue and do the comparison with "/chosen". For other
> errors we should return.
Let's take an example with a path called /deadbeef. This will not hold 
in the variable path. Do you agree that fdt_get_path will return 
-FDT_ERR_NO_SPACE in that case?
AFAIU the function fdt_get_path, the buffer will contain the character
/ followed by garbage as '\0' is only added in successful path.

This also fit with the description of fdt_get_path when 
-FDT_ERR_NO_SPACE. It does not promise you the buffer will contain 
anything. It only tells you that the path on the given node will not fit 
in the buffer.
So I still don't think you can assume the behavior you described above.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel