Re: [PATCH v3 1/5] xen: introduce xen,enhanced dom0less property

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 23/03/2022 00:08, Stefano Stabellini wrote:
> On Sat, 29 Jan 2022, Julien Grall wrote:
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 6931c022a2..9144d6c0b6 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
> > >                                     const struct dt_device_node *node)
> > >    {
> > >        struct kernel_info kinfo = {};
> > > +    const char *dom0less_enhanced;
> > >        int rc;
> > >        u64 mem;
> > >    @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d,
> > >          kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
> > >    +    rc = dt_property_read_string(node, "xen,enhanced",
> > > &dom0less_enhanced);
> > > +    if ( rc == -EILSEQ ||
> > I think the use an -EILSEQ wants an explanation. In a previous version, you
> > wrote that the value would be returned when:
> >
> > fdt set /chosen/domU0 xen,enhanced
> >
> > But it is not clear why. Can you print pp->value, pp->length, strnlen(..) when
> > this happens?
> I added in dt_property_read_string:
>
> printk("DEBUG %s %d value=%s value[0]=%d length=%u 
> len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length, 
> strlen(pp->value));
>
> This is the output:
> (XEN) DEBUG dt_property_read_string 205 value= value[0]=0 length=0 len=0
Thanks posting the log!

For convenience, I am copying the comment on top of 
dt_property_read_string() prototype:
 * Search for a property in a device tree node and retrieve a null
 * terminated string value (pointer to data, not a copy). Returns 0 on
 * success, -EINVAL if the property does not exist, -ENODATA if property
 * doest not have value, and -EILSEQ if the string is not
 * null-terminated with the length of the property data.

Per your log, the length is NULL so I would have assumed -ENODATA would 
be returned. Looking at the implementation:
    const struct dt_property *pp = dt_find_property(np, propname, NULL);

    if ( !pp )
        return -EINVAL;
    if ( !pp->value )
        return -ENODATA;
    if ( strnlen(pp->value, pp->length) >= pp->length )
        return -EILSEQ;

We consider that the property when pp->value is NULL. However, AFAICT, 
we never set pp->value to NULL (see unflatten_dt_node()).
So I think there is a bug in the implementation. I would keep the check 
!pp->value (for hardening purpose) and also return -ENODATA when 
!pp->length.
Most of our device-tree code is from Linux. Looking at v5.17, the bug 
seems to be present there too. This would want to be fixed there too.
Cheers,

--
Julien Grall