[PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[Thomas Huth]

"note_size" can be smaller than sizeof(note), so unconditionally calling
memset(notep, 0, sizeof(note)) could cause a memory corruption here in
case notep has been allocated dynamically, thus let's use note_size as
length argument for memset() instead.

Fixes: 113d8f4e95 ("s390x: pv: Add dump support")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/arch_dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index a2329141e8..a7c44ba49d 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -248,7 +248,7 @@ static int s390x_write_elf64_notes(const char *note_name,
             notep = g_malloc(note_size);
         }
 
-        memset(notep, 0, sizeof(note));
+        memset(notep, 0, note_size);
 
         /* Setup note header data */
         notep->hdr.n_descsz = cpu_to_be32(content_size);
-- 
2.31.1

Re: [PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[Philippe Mathieu-Daudé]

On 14/2/23 15:10, Thomas Huth wrote:
> "note_size" can be smaller than sizeof(note), so unconditionally calling
> memset(notep, 0, sizeof(note)) could cause a memory corruption here in
> case notep has been allocated dynamically, thus let's use note_size as
> length argument for memset() instead.

Correct.

I wonder why use one notep* pointing to a stack allocated or a heap
allocated buffer. This isn't hot path, one heap use could simplify
this code complexity IMO.

> Fixes: 113d8f4e95 ("s390x: pv: Add dump support")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/s390x/arch_dump.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[Thomas Huth]

On 14/02/2023 15.58, Philippe Mathieu-Daudé wrote:
> On 14/2/23 15:10, Thomas Huth wrote:
>> "note_size" can be smaller than sizeof(note), so unconditionally calling
>> memset(notep, 0, sizeof(note)) could cause a memory corruption here in
>> case notep has been allocated dynamically, thus let's use note_size as
>> length argument for memset() instead.
> 
> Correct.
> 
> I wonder why use one notep* pointing to a stack allocated or a heap
> allocated buffer. This isn't hot path, one heap use could simplify
> this code complexity IMO.

You've got a point. I'll give it a try and send a v2.

  Thanks,
    Thomas

Re: [PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[Thomas Huth]

On 15/02/2023 06.20, Thomas Huth wrote:
> On 14/02/2023 15.58, Philippe Mathieu-Daudé wrote:
>> On 14/2/23 15:10, Thomas Huth wrote:
>>> "note_size" can be smaller than sizeof(note), so unconditionally calling
>>> memset(notep, 0, sizeof(note)) could cause a memory corruption here in
>>> case notep has been allocated dynamically, thus let's use note_size as
>>> length argument for memset() instead.
>>
>> Correct.
>>
>> I wonder why use one notep* pointing to a stack allocated or a heap
>> allocated buffer. This isn't hot path, one heap use could simplify
>> this code complexity IMO.
> 
> You've got a point. I'll give it a try and send a v2.

Actually, it looked better as a separate, independent patch, so I sent it as 
"Simplify memory allocation in s390x_write_elf64_notes()" (based on this one 
here).

  Thomas

Re: [PATCH] target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes()

[Janosch Frank]

On 2/14/23 15:10, Thomas Huth wrote:
> "note_size" can be smaller than sizeof(note), so unconditionally calling
> memset(notep, 0, sizeof(note)) could cause a memory corruption here in
> case notep has been allocated dynamically, thus let's use note_size as
> length argument for memset() instead.
> 
> Fixes: 113d8f4e95 ("s390x: pv: Add dump support")
> Signed-off-by: Thomas Huth <thuth@redhat.com>

This was found because a machine was used that has PV support but 
doesn't have the PV dump support (I currently don't have access to a 
previous generation machine). In that case the size of the PV cpu note 
is reported as 0 but it's still being written / processed.

I added proper checks for dump support on my todo list so we can avoid 
writing empty notes. However it's easier said than done since "dump 
support" is actually a combination of KVM, QEMU, the machine AND a bit 
in the SE header that allows dumping. Additionally we need to report the 
size of the notes way before we start the PV dump process where we get 
told if the machine is allowed to dump.

Thanks for helping with the debug effort and creating a patch Thomas!

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Also:
Reported-by: Sebastian Mitterle <smitterl@redhat.com>

> ---
>   target/s390x/arch_dump.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index a2329141e8..a7c44ba49d 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -248,7 +248,7 @@ static int s390x_write_elf64_notes(const char *note_name,
>               notep = g_malloc(note_size);
>           }
>   
> -        memset(notep, 0, sizeof(note));
> +        memset(notep, 0, note_size);
>   
>           /* Setup note header data */
>           notep->hdr.n_descsz = cpu_to_be32(content_size);