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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 25 Mar 2022, Julien Grall wrote:
> 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.

I have added a patch to fix dt_property_read_string. I am about to send
it out as patch of v4 of the series. I'll also follow-up on Linux.

I am thinking of keeping the -EILSEQ in domain_build.c for hardening
purposes.