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

[Julien Grall <julien@xxxxxxx>]

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).
Cheers,

--
Julien Grall