Re: [Xen-devel] [PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 12/12/18 11:53 PM, Stefano Stabellini wrote:
> On Wed, 12 Dec 2018, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 11/12/2018 18:46, Stefano Stabellini wrote:
> > > cplen is unsigned, thus, it can never be < 0. Use a signed integer local
> > > variable instead.
> > The current code checks > 0. Looking at the code I don't think it can ever be
> > negative. So can you details what is the problem you are trying to resolve?
> Yes, it can be "negative" (not actually negative because it is defined
> as unsigned), in fact it happens with the nodes added dynamically by grub
> at boot. This patches fixes booting from grub.
>
> Specifically ilen is initialed to 16, but strlen+1 is 17, so length
> becomes -1. The length of the property generated by grub seems to be
> short by 1.
Such explanation should have been in the commit message, it helps to 
diagnostics where the problem is coming from.
Looking at the specification [1] section 2.3.1, a compatible property is 
a concatenated list of null terminated strings. So the current code is 
actually correct and there is a bug in GRUB.
This bug was actually fixed by commit ae5817f1d "arm64/xen_boot: Fix Xen 
boot using GRUB2 on AARCH64".
I assume you are using Grub 2.02 which does not contain the full support 
for Xen. So I would not even consider to work-around it in Xen.
Instead, I would recommend you to upgrade to a more recent GRUB. It 
would be more likely staging as there are no new GRUB version.
Another solution is to use chainloading.

Cheers,

[1] 
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2

> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > ---
> > >    xen/common/device_tree.c | 7 +++++--
> > >    1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index 8fc401d..df274cc 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -213,17 +213,20 @@ bool_t dt_device_is_compatible(const struct
> > > dt_device_node *device,
> > >    {
> > >        const char* cp;
> > >        u32 cplen, l;
> > > +    s64 ilen;
> > >          cp = dt_get_property(device, "compatible", &cplen);
> > >        if ( cp == NULL )
> > >            return 0;
> > > -    while ( cplen > 0 )
> > > +
> > > +    ilen = cplen;
> > > +    while ( ilen > 0 )
> > >        {
> > >            if ( dt_compat_cmp(cp, compat) == 0 )
> > >                return 1;
> > >            l = strlen(cp) + 1;
> > >            cp += l;
> > > -        cplen -= l;
> > > +        ilen -= l;
> > >        }
> > >          return 0;
> > --
> > Julien Grall
--
Julien Grall

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