qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hw/elf_ops: clear uninitialized segment space
@ 2021-04-14 10:58 Laurent Vivier
  2021-04-14 12:16 ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2021-04-14 10:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Philippe Mathieu-Daudé,
	Laurent Vivier, Stefano Garzarella

When the mem_size of the segment is bigger than the file_size,
and if this space doesn't overlap another segment, it needs
to be cleared.

This bug is very similar to the one we had for linux-user,
22d113b52f41 ("linux-user: Fix loading of BSS segments"),
where .bss section is encoded as an extension of the the data
one by setting the segment p_memsz > p_filesz.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 include/hw/elf_ops.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 6ee458e7bc3c..e3dcee3ee349 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -562,6 +562,23 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                     if (res != MEMTX_OK) {
                         goto fail;
                     }
+                    /*
+                     * We need to zero'ify the space that is not copied
+                     * from file
+                     */
+                    if (file_size < mem_size) {
+                        static uint8_t zero[4096];
+                        uint64_t i;
+                        for (i = file_size; i < mem_size; i += sizeof(zero)) {
+                            res = address_space_write(
+                                         as ? as : &address_space_memory,
+                                         addr + i, MEMTXATTRS_UNSPECIFIED,
+                                         zero, MIN(sizeof(zero), mem_size - i));
+                            if (res != MEMTX_OK) {
+                                goto fail;
+                            }
+                        }
+                    }
                 }
             }
 
-- 
2.30.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/elf_ops: clear uninitialized segment space
  2021-04-14 10:58 [PATCH] hw/elf_ops: clear uninitialized segment space Laurent Vivier
@ 2021-04-14 12:16 ` Philippe Mathieu-Daudé
  2021-04-14 12:41   ` Laurent Vivier
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-14 12:16 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel; +Cc: Peter Maydell, Stefano Garzarella

On 4/14/21 12:58 PM, Laurent Vivier wrote:
> When the mem_size of the segment is bigger than the file_size,
> and if this space doesn't overlap another segment, it needs
> to be cleared.
> 
> This bug is very similar to the one we had for linux-user,
> 22d113b52f41 ("linux-user: Fix loading of BSS segments"),
> where .bss section is encoded as an extension of the the data
> one by setting the segment p_memsz > p_filesz.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/hw/elf_ops.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 6ee458e7bc3c..e3dcee3ee349 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -562,6 +562,23 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      if (res != MEMTX_OK) {
>                          goto fail;
>                      }
> +                    /*
> +                     * We need to zero'ify the space that is not copied
> +                     * from file
> +                     */
> +                    if (file_size < mem_size) {
> +                        static uint8_t zero[4096];

Given it is unlikely, maybe better use:

              g_autofree uint8_t *zero = g_new0(uint8_t, 4096);

> +                        uint64_t i;
> +                        for (i = file_size; i < mem_size; i += sizeof(zero)) {
> +                            res = address_space_write(
> +                                         as ? as : &address_space_memory,
> +                                         addr + i, MEMTXATTRS_UNSPECIFIED,
> +                                         zero, MIN(sizeof(zero), mem_size - i));
> +                            if (res != MEMTX_OK) {
> +                                goto fail;
> +                            }
> +                        }
> +                    }
>                  }
>              }
>  
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/elf_ops: clear uninitialized segment space
  2021-04-14 12:16 ` Philippe Mathieu-Daudé
@ 2021-04-14 12:41   ` Laurent Vivier
  2021-04-15  8:44     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2021-04-14 12:41 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Peter Maydell, Stefano Garzarella

Le 14/04/2021 à 14:16, Philippe Mathieu-Daudé a écrit :
> On 4/14/21 12:58 PM, Laurent Vivier wrote:
>> When the mem_size of the segment is bigger than the file_size,
>> and if this space doesn't overlap another segment, it needs
>> to be cleared.
>>
>> This bug is very similar to the one we had for linux-user,
>> 22d113b52f41 ("linux-user: Fix loading of BSS segments"),
>> where .bss section is encoded as an extension of the the data
>> one by setting the segment p_memsz > p_filesz.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  include/hw/elf_ops.h | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index 6ee458e7bc3c..e3dcee3ee349 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>> @@ -562,6 +562,23 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>                      if (res != MEMTX_OK) {
>>                          goto fail;
>>                      }
>> +                    /*
>> +                     * We need to zero'ify the space that is not copied
>> +                     * from file
>> +                     */
>> +                    if (file_size < mem_size) {
>> +                        static uint8_t zero[4096];
> 
> Given it is unlikely, maybe better use:
> 
>               g_autofree uint8_t *zero = g_new0(uint8_t, 4096);
> 
>

I don't know what is the best solution but this seems to introduce a lot of complexity only to have
a page of 0s.

Thanks,
Laurent



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/elf_ops: clear uninitialized segment space
  2021-04-14 12:41   ` Laurent Vivier
@ 2021-04-15  8:44     ` Philippe Mathieu-Daudé
  2021-04-15  9:57       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15  8:44 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel; +Cc: Peter Maydell, Stefano Garzarella

On 4/14/21 2:41 PM, Laurent Vivier wrote:
> Le 14/04/2021 à 14:16, Philippe Mathieu-Daudé a écrit :
>> On 4/14/21 12:58 PM, Laurent Vivier wrote:
>>> When the mem_size of the segment is bigger than the file_size,
>>> and if this space doesn't overlap another segment, it needs
>>> to be cleared.
>>>
>>> This bug is very similar to the one we had for linux-user,
>>> 22d113b52f41 ("linux-user: Fix loading of BSS segments"),
>>> where .bss section is encoded as an extension of the the data
>>> one by setting the segment p_memsz > p_filesz.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>  include/hw/elf_ops.h | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>>> index 6ee458e7bc3c..e3dcee3ee349 100644
>>> --- a/include/hw/elf_ops.h
>>> +++ b/include/hw/elf_ops.h
>>> @@ -562,6 +562,23 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>>                      if (res != MEMTX_OK) {
>>>                          goto fail;
>>>                      }
>>> +                    /*
>>> +                     * We need to zero'ify the space that is not copied
>>> +                     * from file
>>> +                     */
>>> +                    if (file_size < mem_size) {
>>> +                        static uint8_t zero[4096];
>>
>> Given it is unlikely, maybe better use:
>>
>>               g_autofree uint8_t *zero = g_new0(uint8_t, 4096);
> 
> I don't know what is the best solution but this seems to introduce a lot of complexity only to have
> a page of 0s.

Less complex alternative is to use dma_memory_set():

   dma_memory_set(as, file_size, 0, mem_size - file_size);

Actually we should extract address_space_set() from it, keeping
the dma_barrier() call in dma_memory_set(), and use address_space_set()
here.

What do you think?

Phil.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/elf_ops: clear uninitialized segment space
  2021-04-15  8:44     ` Philippe Mathieu-Daudé
@ 2021-04-15  9:57       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15  9:57 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel; +Cc: Peter Maydell, Stefano Garzarella

On 4/15/21 10:44 AM, Philippe Mathieu-Daudé wrote:
> On 4/14/21 2:41 PM, Laurent Vivier wrote:
>> Le 14/04/2021 à 14:16, Philippe Mathieu-Daudé a écrit :
>>> On 4/14/21 12:58 PM, Laurent Vivier wrote:
>>>> When the mem_size of the segment is bigger than the file_size,
>>>> and if this space doesn't overlap another segment, it needs
>>>> to be cleared.
>>>>
>>>> This bug is very similar to the one we had for linux-user,
>>>> 22d113b52f41 ("linux-user: Fix loading of BSS segments"),
>>>> where .bss section is encoded as an extension of the the data
>>>> one by setting the segment p_memsz > p_filesz.
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>>  include/hw/elf_ops.h | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>>>> index 6ee458e7bc3c..e3dcee3ee349 100644
>>>> --- a/include/hw/elf_ops.h
>>>> +++ b/include/hw/elf_ops.h
>>>> @@ -562,6 +562,23 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>>>                      if (res != MEMTX_OK) {
>>>>                          goto fail;
>>>>                      }
>>>> +                    /*
>>>> +                     * We need to zero'ify the space that is not copied
>>>> +                     * from file
>>>> +                     */
>>>> +                    if (file_size < mem_size) {
>>>> +                        static uint8_t zero[4096];
>>>
>>> Given it is unlikely, maybe better use:
>>>
>>>               g_autofree uint8_t *zero = g_new0(uint8_t, 4096);
>>
>> I don't know what is the best solution but this seems to introduce a lot of complexity only to have
>> a page of 0s.
> 
> Less complex alternative is to use dma_memory_set():
> 
>    dma_memory_set(as, file_size, 0, mem_size - file_size);
> 
> Actually we should extract address_space_set() from it, keeping
> the dma_barrier() call in dma_memory_set(), and use address_space_set()
> here.
> 
> What do you think?

I'll post a patch.



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-04-15  9:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 10:58 [PATCH] hw/elf_ops: clear uninitialized segment space Laurent Vivier
2021-04-14 12:16 ` Philippe Mathieu-Daudé
2021-04-14 12:41   ` Laurent Vivier
2021-04-15  8:44     ` Philippe Mathieu-Daudé
2021-04-15  9:57       ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).