diff mbox series

[2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times

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

Commit Message

Julien Grall April 2, 2021, 3:21 p.m. UTC 
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>
---
 xen/common/gunzip.c  | 5 +++++
 xen/common/inflate.c | 6 ++++++
 2 files changed, 11 insertions(+)

Comments

Jan Beulich April 6, 2021, 7:40 a.m. UTC | #1 
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>
Jan Beulich April 7, 2021, 10:39 a.m. UTC | #2 
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
Julien Grall April 7, 2021, 6:18 p.m. UTC | #3 
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 mbox series

Patch

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;