[SeaBIOS] [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table

Daniel P. Berrangé posted 1 patch 3 years, 7 months ago
Failed in applying to current master (apply log)
src/fw/biostables.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
[SeaBIOS] [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table
Posted by Daniel P. Berrangé 3 years, 7 months ago
SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
maximum length of 0xffff. If the SMBIOS data received from QEMU is large
enough, then adding the type 0 table will cause integer overflow. This
results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/fw/biostables.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/fw/biostables.c b/src/fw/biostables.c
index 0c07833..794b5be 100644
--- a/src/fw/biostables.c
+++ b/src/fw/biostables.c
@@ -462,10 +462,16 @@ smbios_romfile_setup(void)
         /* common case: add our own type 0, with 3 strings and 4 '\0's */
         u16 t0_len = sizeof(struct smbios_type_0) + strlen(BIOS_NAME) +
                      strlen(VERSION) + strlen(BIOS_DATE) + 4;
-        ep.structure_table_length += t0_len;
-        if (t0_len > ep.max_structure_size)
-            ep.max_structure_size = t0_len;
-        ep.number_of_structures++;
+        if (t0_len > (0xffff - ep.structure_table_length)) {
+            dprintf(1, "Insufficient space (%d bytes) to add SMBIOS type 0 table (%d bytes)\n",
+                    0xffff - ep.structure_table_length, t0_len);
+            need_t0 = 0;
+        } else {
+            ep.structure_table_length += t0_len;
+            if (t0_len > ep.max_structure_size)
+                ep.max_structure_size = t0_len;
+            ep.number_of_structures++;
+        }
     }
 
     /* allocate final blob and record its address in the entry point */
-- 
2.26.2
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table
Posted by Kevin O'Connor 3 years, 7 months ago
On Tue, Sep 08, 2020 at 04:21:03PM +0100, Daniel P. Berrangé wrote:
> SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
> maximum length of 0xffff. If the SMBIOS data received from QEMU is large
> enough, then adding the type 0 table will cause integer overflow. This
> results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.

Thanks.  The patch looks fine to me.  However, when I run "git am" on
your email, it's not taking the patch.  (Perhaps the email whitespace
got corrupted?)

==============
Applying: smbios: avoid integer overflow adding SMBIOS type 0 table
error: patch failed: src/fw/biostables.c:462
error: src/fw/biostables.c: patch does not apply
Patch failed at 0001 smbios: avoid integer overflow adding SMBIOS type 0 table
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
==============

-Kevin

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/fw/biostables.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> index 0c07833..794b5be 100644
> --- a/src/fw/biostables.c
> +++ b/src/fw/biostables.c
> @@ -462,10 +462,16 @@ smbios_romfile_setup(void)
>          /* common case: add our own type 0, with 3 strings and 4 '\0's */
>          u16 t0_len = sizeof(struct smbios_type_0) + strlen(BIOS_NAME) +
>                       strlen(VERSION) + strlen(BIOS_DATE) + 4;
> -        ep.structure_table_length += t0_len;
> -        if (t0_len > ep.max_structure_size)
> -            ep.max_structure_size = t0_len;
> -        ep.number_of_structures++;
> +        if (t0_len > (0xffff - ep.structure_table_length)) {
> +            dprintf(1, "Insufficient space (%d bytes) to add SMBIOS type 0 table (%d bytes)\n",
> +                    0xffff - ep.structure_table_length, t0_len);
> +            need_t0 = 0;
> +        } else {
> +            ep.structure_table_length += t0_len;
> +            if (t0_len > ep.max_structure_size)
> +                ep.max_structure_size = t0_len;
> +            ep.number_of_structures++;
> +        }
>      }
>  
>      /* allocate final blob and record its address in the entry point */
> -- 
> 2.26.2
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table
Posted by Daniel P. Berrangé 3 years, 7 months ago
On Fri, Sep 11, 2020 at 02:03:23PM -0400, Kevin O'Connor wrote:
> On Tue, Sep 08, 2020 at 04:21:03PM +0100, Daniel P. Berrangé wrote:
> > SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
> > maximum length of 0xffff. If the SMBIOS data received from QEMU is large
> > enough, then adding the type 0 table will cause integer overflow. This
> > results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.
> 
> Thanks.  The patch looks fine to me.  However, when I run "git am" on
> your email, it's not taking the patch.  (Perhaps the email whitespace
> got corrupted?)
> 
> ==============
> Applying: smbios: avoid integer overflow adding SMBIOS type 0 table
> error: patch failed: src/fw/biostables.c:462
> error: src/fw/biostables.c: patch does not apply
> Patch failed at 0001 smbios: avoid integer overflow adding SMBIOS type 0 table
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> ==============

This was just sent using  git send-email, so I can't see what would
corrupt it on the sending side. In any case, I've got a copy on github
you can pull from, on my "smbios-len" branch, this commit:

https://github.com/berrange/seabios/commit/4ea6aa9471f79cc81f957d6c0e2bb238d24675e5


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 :|
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table
Posted by Kevin O'Connor 3 years, 7 months ago
On Mon, Sep 14, 2020 at 10:38:26AM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 11, 2020 at 02:03:23PM -0400, Kevin O'Connor wrote:
> > On Tue, Sep 08, 2020 at 04:21:03PM +0100, Daniel P. Berrangé wrote:
> > > SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
> > > maximum length of 0xffff. If the SMBIOS data received from QEMU is large
> > > enough, then adding the type 0 table will cause integer overflow. This
> > > results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.
> > 
> > Thanks.  The patch looks fine to me.  However, when I run "git am" on
> > your email, it's not taking the patch.  (Perhaps the email whitespace
> > got corrupted?)
> > 
> > ==============
> > Applying: smbios: avoid integer overflow adding SMBIOS type 0 table
> > error: patch failed: src/fw/biostables.c:462
> > error: src/fw/biostables.c: patch does not apply
> > Patch failed at 0001 smbios: avoid integer overflow adding SMBIOS type 0 table
> > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> > ==============
> 
> This was just sent using  git send-email, so I can't see what would
> corrupt it on the sending side. In any case, I've got a copy on github
> you can pull from, on my "smbios-len" branch, this commit:
> 
> https://github.com/berrange/seabios/commit/4ea6aa9471f79cc81f957d6c0e2bb238d24675e5

Okay, thanks, I committed that change.  I'm not sure what went wrong
with my email.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table
Posted by Philippe Mathieu-Daudé 3 years, 7 months ago
On 9/8/20 5:21 PM, Daniel P. Berrangé wrote:
> SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
> maximum length of 0xffff. If the SMBIOS data received from QEMU is large
> enough, then adding the type 0 table will cause integer overflow. This
> results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/fw/biostables.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> index 0c07833..794b5be 100644
> --- a/src/fw/biostables.c
> +++ b/src/fw/biostables.c
> @@ -462,10 +462,16 @@ smbios_romfile_setup(void)
>          /* common case: add our own type 0, with 3 strings and 4 '\0's */
>          u16 t0_len = sizeof(struct smbios_type_0) + strlen(BIOS_NAME) +
>                       strlen(VERSION) + strlen(BIOS_DATE) + 4;
> -        ep.structure_table_length += t0_len;
> -        if (t0_len > ep.max_structure_size)
> -            ep.max_structure_size = t0_len;
> -        ep.number_of_structures++;
> +        if (t0_len > (0xffff - ep.structure_table_length)) {
> +            dprintf(1, "Insufficient space (%d bytes) to add SMBIOS type 0 table (%d bytes)\n",
> +                    0xffff - ep.structure_table_length, t0_len);
> +            need_t0 = 0;
> +        } else {
> +            ep.structure_table_length += t0_len;
> +            if (t0_len > ep.max_structure_size)
> +                ep.max_structure_size = t0_len;
> +            ep.number_of_structures++;
> +        }
>      }
>  
>      /* allocate final blob and record its address in the entry point */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org