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
