[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.