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

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 12/11/2018 21:13, Stefano Stabellini wrote:
> 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.

The problem is not really because of '\0' is missing but the fact you 
may not have 8 valid characters in the buffer. In some case, you will 
only have one, yet you compare with a 8 characters string.

> 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, 

I would have been happy to just update the documentation but the code 
does not seem match that behavior (see above). If you can prove me the 
case I gave can work perfectly, then I might reconsider it.

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.

I would prefer the 92 characters with a comment on top. This is quite 
unlikely to hit it.

Cheers,

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