[PATCH] smbios: Fix buffer overrun when using path= option

[PATCH] smbios: Fix buffer overrun when using path= option

[Daan De Meyer]

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

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Thomas Huth]

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

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Daniel P. Berrangé]

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 :|

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Daniel P. Berrangé]

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
-- 
|: 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 :|

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Philippe Mathieu-Daudé]

+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);

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Daan De Meyer]

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
>

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Philippe Mathieu-Daudé]

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
>>
> 

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Valentin David]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 611 bytes --]

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Philippe Mathieu-Daudé]

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>

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Valentin David]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

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>
>
>

[-- Attachment #2: Type: text/html, Size: 1143 bytes --]

Re: [PATCH] smbios: Fix buffer overrun when using path= option

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]