| Message ID | 20250323213622.2581013-1-daan.j.demeyer@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | smbios: Fix buffer overrun when using path= option | expand |
On 23/03/2025 22.35, Daan De Meyer wrote: > We have to make sure the array of bytes read from the path= file > is null-terminated, otherwise we run into a buffer overrun later on. > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > --- > hw/smbios/smbios.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 02a09eb9cd..ad4cd6721e 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > g_byte_array_append(data, (guint8 *)buf, ret); > } > > + buf[0] = '\0'; > + g_byte_array_append(data, (guint8 *)buf, 1); > + > qemu_close(fd); > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); Please make sure to put the maintainers on CC: (done now, for the next time please see the MAINTAINERS file or use the scripts/get_maintainers.pl script), otherwise your patch might go unnoticed. Thomas
On Sun, Mar 23, 2025 at 10:35:54PM +0100, Daan De Meyer wrote: > We have to make sure the array of bytes read from the path= file > is null-terminated, otherwise we run into a buffer overrun later on. > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > --- > hw/smbios/smbios.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 02a09eb9cd..ad4cd6721e 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > g_byte_array_append(data, (guint8 *)buf, ret); > } > > + buf[0] = '\0'; > + g_byte_array_append(data, (guint8 *)buf, 1); > + > qemu_close(fd); > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
CC qemu-stable - this needs cherry-picking into all active stable branches once accepted. On Mon, Mar 24, 2025 at 09:12:53AM +0000, Daniel P. Berrangé wrote: > On Sun, Mar 23, 2025 at 10:35:54PM +0100, Daan De Meyer wrote: > > We have to make sure the array of bytes read from the path= file > > is null-terminated, otherwise we run into a buffer overrun later on. > > > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > > --- > > hw/smbios/smbios.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > > index 02a09eb9cd..ad4cd6721e 100644 > > --- a/hw/smbios/smbios.c > > +++ b/hw/smbios/smbios.c > > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > > g_byte_array_append(data, (guint8 *)buf, ret); > > } > > > > + buf[0] = '\0'; > > + g_byte_array_append(data, (guint8 *)buf, 1); > > + > > qemu_close(fd); > > > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > With regards, Daniel
+Valentin On 23/3/25 22:35, Daan De Meyer wrote: > We have to make sure the array of bytes read from the path= file > is null-terminated, otherwise we run into a buffer overrun later on. > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > --- > hw/smbios/smbios.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 02a09eb9cd..ad4cd6721e 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > g_byte_array_append(data, (guint8 *)buf, ret); > } > > + buf[0] = '\0'; > + g_byte_array_append(data, (guint8 *)buf, 1); > + > qemu_close(fd); > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
Hi, Unless I'm missing something, I don't think the patch has been merged yet. Any chance it might have been missed? Cheers, Daan On Sun, 23 Mar 2025 at 22:36, Daan De Meyer <daan.j.demeyer@gmail.com> wrote: > > We have to make sure the array of bytes read from the path= file > is null-terminated, otherwise we run into a buffer overrun later on. > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > --- > hw/smbios/smbios.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 02a09eb9cd..ad4cd6721e 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > g_byte_array_append(data, (guint8 *)buf, ret); > } > > + buf[0] = '\0'; > + g_byte_array_append(data, (guint8 *)buf, 1); > + > qemu_close(fd); > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); > -- > 2.49.0 >
Hi Daan, On 3/4/25 21:29, Daan De Meyer wrote: > Hi, > > Unless I'm missing something, I don't think the patch has been merged > yet. Any chance it might have been missed? I have it tagged, as sensible enough, in case nobody else takes it. IIRC it was sent the same day I posted my latest pull request, so it'd go in the next one, due before next Tuesday. Also I was hoping I could get feedback from Valentin. > > Cheers, > > Daan > > On Sun, 23 Mar 2025 at 22:36, Daan De Meyer <daan.j.demeyer@gmail.com> wrote: >> >> We have to make sure the array of bytes read from the path= file >> is null-terminated, otherwise we run into a buffer overrun later on. >> >> Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 >> >> Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> >> --- >> hw/smbios/smbios.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c >> index 02a09eb9cd..ad4cd6721e 100644 >> --- a/hw/smbios/smbios.c >> +++ b/hw/smbios/smbios.c >> @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, >> g_byte_array_append(data, (guint8 *)buf, ret); >> } >> >> + buf[0] = '\0'; >> + g_byte_array_append(data, (guint8 *)buf, 1); >> + >> qemu_close(fd); >> >> *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); >> -- >> 2.49.0 >> >
On Thu, Apr 3, 2025 at 9:37 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Also I was hoping I could get feedback from Valentin. > > Sorry, I did not realize that you wanted my feedback. Daan's patch looks fine to me. I have manually tested it and it fixes my issue.
On 4/4/25 16:46, Valentin David wrote: > On Thu, Apr 3, 2025 at 9:37 PM Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> wrote: > > Also I was hoping I could get feedback from Valentin. > > > Sorry, I did not realize that you wanted my feedback. Daan's patch looks > fine to me. I have manually tested it and it fixes my issue. Thanks! Might I add your tag then? Tested-by: Valentin David <valentin.david@canonical.com>
Yes. On Fri, Apr 4, 2025 at 5:02 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 4/4/25 16:46, Valentin David wrote: > > On Thu, Apr 3, 2025 at 9:37 PM Philippe Mathieu-Daudé <philmd@linaro.org > > <mailto:philmd@linaro.org>> wrote: > > > > Also I was hoping I could get feedback from Valentin. > > > > > > Sorry, I did not realize that you wanted my feedback. Daan's patch looks > > fine to me. I have manually tested it and it fixes my issue. > > Thanks! Might I add your tag then? > > Tested-by: Valentin David <valentin.david@canonical.com> > >
On Mon, Mar 24, 2025 at 07:24:59AM +0100, Thomas Huth wrote: > On 23/03/2025 22.35, Daan De Meyer wrote: > > We have to make sure the array of bytes read from the path= file > > is null-terminated, otherwise we run into a buffer overrun later on. > > > > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 > > > > Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> > > --- > > hw/smbios/smbios.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > > index 02a09eb9cd..ad4cd6721e 100644 > > --- a/hw/smbios/smbios.c > > +++ b/hw/smbios/smbios.c > > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, > > g_byte_array_append(data, (guint8 *)buf, ret); > > } > > + buf[0] = '\0'; > > + g_byte_array_append(data, (guint8 *)buf, 1); > > + > > qemu_close(fd); > > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1); > > Please make sure to put the maintainers on CC: (done now, for the next time > please see the MAINTAINERS file or use the scripts/get_maintainers.pl > script), otherwise your patch might go unnoticed. Michael, Igor, Ani: This patch is needed for QEMU 10.0. You are the maintainers, please review this patch. Thanks! Stefan
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index 02a09eb9cd..ad4cd6721e 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque, g_byte_array_append(data, (guint8 *)buf, ret); } + buf[0] = '\0'; + g_byte_array_append(data, (guint8 *)buf, 1); + qemu_close(fd); *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
We have to make sure the array of bytes read from the path= file is null-terminated, otherwise we run into a buffer overrun later on. Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support loading OEM strings values from a file") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879 Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> --- hw/smbios/smbios.c | 3 +++ 1 file changed, 3 insertions(+)