Re: [PATCH v2 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 02/02/2023 12:23, Julien Grall wrote:
> 
> 
> On 02/02/2023 11:12, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>>
>> On 02/02/2023 12:01, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 02/02/2023 08:49, Michal Orzel wrote:
>>>> @@ -265,11 +284,14 @@ static __init int kernel_decompress(struct 
>>>> bootmodule *mod)
>>>>    #define IH_ARCH_ARM             2       /* ARM          */
>>>>    #define IH_ARCH_ARM64           22      /* ARM64        */
>>>>
>>>> +/* uImage Compression Types */
>>>> +#define IH_COMP_GZIP            1
>>>> +
>>>>    /*
>>>>     * Check if the image is a uImage and setup kernel_info
>>>>     */
>>>>    static int __init kernel_uimage_probe(struct kernel_info *info,
>>>> -                                      paddr_t addr, paddr_t size)
>>>> +                                      struct bootmodule *mod)
>>>>    {
>>>>        struct {
>>>>            __be32 magic;   /* Image Header Magic Number */
>>>> @@ -287,6 +309,8 @@ static int __init kernel_uimage_probe(struct 
>>>> kernel_info *info,
>>>>        } uimage;
>>>>
>>>>        uint32_t len;
>>>> +    paddr_t addr = mod->start;
>>>> +    paddr_t size = mod->size;
>>>>
>>>>        if ( size < sizeof(uimage) )
>>>>            return -EINVAL;
>>>
>>> Shouldn't we return -ENOENT here?
>> Frankly speaking, I do not want to fall through in such a case.
>> If a kernel size is less than 64B, something is wrong, isn't it?
> 
> I agree something is likely wrong but this should not be the job of
> kernel_uimage_probe() to enforce this for everyone.
> 
> To give a concrete example, let's imagine we decide to re-order the call
> so kernel_uimage_probe() happens *after* an new header A than would
> require 128 bytes (the number is made up).
> 
> It would be wrong for A to return -EINVAL because the other protocol may
> require a smaller size. The same goes here even at least for consistency.
> 
> So if you really want to enforce a minimum size, then such check should
> be in the caller. Then each probe could return -ENOENT if the size is
> too small.
> 
>> I am not sure if Xen would handle a kernel whose size is less than a page.
> 
> I don't see any reason why it would not be.
> 
>>
>> I do not like the whole falling through in kernel_probe even in case of 
>> obvious violations.
>> But this is something not related to this series so I added to my TODO to 
>> properly handle
>> the return types from kernel_probe path. If you really think, we should 
>> return -ENOENT here,
>> then ok (although I do not like it). Could this be done on commit if you 
>> insist on that?
> 
> See above for an alternative proposal. Depending on the version we
> settle on I can do it on commit (but this is not going to happen today
> as OSSTEst is still blocked).
Ok, lets stick to your original suggestion with s/-EINVAL/-ENOENT/
and I will come up with something for a future patch as this will require more 
changes
to make it generic. I also do not like at all the fact that we are not 
informing the user about the error code
when calling panic from construct_{dom0,domU}...

> 
> Cheers,
> 
> --
> Julien Grall

~Michal