| Message ID | 20230214141056.680969-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/s390x/arch_dump: Fix memory corruption in s390x_write_elf64_notes() | expand |
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>
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);
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
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
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);
"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(-)