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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 9 Nov 2018, Julien Grall wrote:
> 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.

The lack of '\0' is not an issue, we can still compare the content with
strncmp even if '\0' is missing. But you are right, the description is
not clear about the content of `path' if -FDT_ERR_NO_SPACE is returned.
It doesn't clearly say that the content is still guaranteed to be
properly populated until the end of `buf'.

If we don't want to rely on the implementation of fdt_get_path to still
populate the content in case of -FDT_ERR_NO_SPACE, then we'll have to
allocate roughtly DT_MAX_NAME*3 = 41*3 chars for path.
Technically I think it would be DT_MAX_NAME*2+6 ("chosen") +3 ('/' three
times) + ('\0') = 92. We could use 100 chars to stay on the safe side.
Is that OK for you?

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