Message ID | 20210402152105.29387-3-julien@xen.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xen/arm: Couple of bug fixes when decompressing kernels | expand |
On 02.04.2021 17:21, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Currently perform_gunzip() can only be called once because the > the internal state (e.g allocate) is not fully re-initialized. > > This works fine if you are only booting dom0. But this will break when > booting multiple using the dom0less that uses compressed kernel images. > > This can be resolved by re-initializing bytes_out, malloc_ptr, > malloc_count every time perform_gunzip() is called. > > Note the latter is only re-initialized for hardening purpose as there is > no guarantee that every malloc() are followed by free() (It should in > theory!). > > Take the opportunity to check the return of alloc_heap_pages() to return > an error rather than dereferencing a NULL pointer later on failure. > > Reported-by: Charles Chiou <cchiou@ambarella.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 02.04.2021 17:21, Julien Grall wrote: > --- a/xen/common/inflate.c > +++ b/xen/common/inflate.c > @@ -238,6 +238,12 @@ STATIC const ush mask_bits[] = { > static unsigned long INITDATA malloc_ptr; > static int INITDATA malloc_count; > > +static void init_allocator(void) > +{ > + malloc_ptr = free_mem_ptr; > + malloc_count = 0; > +} > + > static void *INIT malloc(int size) > { > void *p; I'm sorry for noticing this only now, but I'm afraid this may break the build in certain environments / configurations. Iirc clang is relatively likely to not inline functions in debug builds even when they're used just once. Yet when the new function doesn't end up getting inlined, it needs INIT added or else the cmd_obj_init_o checking would find a non-empty .text section. (If there's no actual build breakage anywhere, I can also address this in my to-be-re-based "gunzip: drop INIT{,DATA} and STATIC", which is intended to go in as soon as the tree is fully open again.) Jan
Hi Jan, On 07/04/2021 11:39, Jan Beulich wrote: > On 02.04.2021 17:21, Julien Grall wrote: >> --- a/xen/common/inflate.c >> +++ b/xen/common/inflate.c >> @@ -238,6 +238,12 @@ STATIC const ush mask_bits[] = { >> static unsigned long INITDATA malloc_ptr; >> static int INITDATA malloc_count; >> >> +static void init_allocator(void) >> +{ >> + malloc_ptr = free_mem_ptr; >> + malloc_count = 0; >> +} >> + >> static void *INIT malloc(int size) >> { >> void *p; > > I'm sorry for noticing this only now, but I'm afraid this may break > the build in certain environments / configurations. You actually mentioned it on the original thread that reported the bug. But I forgot to add INIT. Sorry for that :(. > Iirc clang is > relatively likely to not inline functions in debug builds even when > they're used just once. Yet when the new function doesn't end up > getting inlined, it needs INIT added or else the cmd_obj_init_o > checking would find a non-empty .text section. (If there's no actual > build breakage anywhere, I can also address this in my to-be-re-based > "gunzip: drop INIT{,DATA} and STATIC", which is intended to go in as > soon as the tree is fully open again.) The pipeline actually reported some failure. But I initially didn't notice them. I think we want to fix it because your rework, so I will send a patch. Sorry for the breakage. Cheers,
diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c index db4efcd34b77..425d64e904d8 100644 --- a/xen/common/gunzip.c +++ b/xen/common/gunzip.c @@ -114,11 +114,16 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len) window = (unsigned char *)output; free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0); + if ( !free_mem_ptr ) + return -ENOMEM; + free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER); + init_allocator(); inbuf = (unsigned char *)image; insize = image_len; inptr = 0; + bytes_out = 0; makecrc(); diff --git a/xen/common/inflate.c b/xen/common/inflate.c index f99c985d6135..d8c28a3e9593 100644 --- a/xen/common/inflate.c +++ b/xen/common/inflate.c @@ -238,6 +238,12 @@ STATIC const ush mask_bits[] = { static unsigned long INITDATA malloc_ptr; static int INITDATA malloc_count; +static void init_allocator(void) +{ + malloc_ptr = free_mem_ptr; + malloc_count = 0; +} + static void *INIT malloc(int size) { void *p;