On 02/02/2023 11:36, Michal Orzel wrote:
Hi Julien,
Hi,
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/
It looks like I didn't gave my reviewed-by. So:
Reviewed-by: Julien Grall <jgr...@amazon.com>
I will change it on commit. As we discussed on IRC, even if there is no
arm32 testing in OSSTest, I expect this to only impact boot and
therefore should be caught by the Gitlab CI.
You already done some testing [1], so I will commit it now.
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}...
I saw you posted it. I will add it in my queue of patches to review.
Cheers,
[1]
https://gitlab.com/xen-project/people/morzel/xen-orzelmichal/-/pipelines/770984105
--
Julien Grall
