Both, "rom->addr" and "addr" are derived from the binary image
that can be loaded with the "-kernel" paramer. The code in
rom_copy() then calculates:
d = dest + (rom->addr - addr);
and uses "d" as destination in a memcpy() some lines later. Now with
bad kernel images, it is possible that rom->addr is smaller than addr,
thus "rom->addr - addr" gets negative and the memcpy() then tries to
copy contents from the image to a bad memory location. In the best case,
this just crashes QEMU, in the worst case, this could maybe be used to
inject code from the kernel image into the QEMU binary, so we better fix
it with an additional sanity check here.
Cc: qemu-stable@nongnu.org
Reported-by: Guangming Liu
Buglink: https://bugs.launchpad.net/qemu/+bug/1844635
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/core/loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0d60219364..5099f27dc8 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
if (rom->addr + rom->romsize < addr) {
continue;
}
- if (rom->addr > end) {
+ if (rom->addr > end || rom->addr < addr) {
break;
}
--
2.18.1
On Wed, Sep 25, 2019 at 03:03:31PM +0200, Thomas Huth wrote: > Both, "rom->addr" and "addr" are derived from the binary image > that can be loaded with the "-kernel" paramer. The code in > rom_copy() then calculates: > > d = dest + (rom->addr - addr); > > and uses "d" as destination in a memcpy() some lines later. Now with > bad kernel images, it is possible that rom->addr is smaller than addr, > thus "rom->addr - addr" gets negative and the memcpy() then tries to > copy contents from the image to a bad memory location. In the best case, > this just crashes QEMU, in the worst case, this could maybe be used to > inject code from the kernel image into the QEMU binary, so we better fix > it with an additional sanity check here. > > Cc: qemu-stable@nongnu.org > Reported-by: Guangming Liu > Buglink: https://bugs.launchpad.net/qemu/+bug/1844635 > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/core/loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 0d60219364..5099f27dc8 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size) > if (rom->addr + rom->romsize < addr) { > continue; > } > - if (rom->addr > end) { > + if (rom->addr > end || rom->addr < addr) { > break; > } > > -- > 2.18.1
On 25/09/19 15:22, Michael S. Tsirkin wrote: >> Both, "rom->addr" and "addr" are derived from the binary image >> that can be loaded with the "-kernel" paramer. The code in >> rom_copy() then calculates: >> >> d = dest + (rom->addr - addr); >> >> and uses "d" as destination in a memcpy() some lines later. Now with >> bad kernel images, it is possible that rom->addr is smaller than addr, >> thus "rom->addr - addr" gets negative and the memcpy() then tries to >> copy contents from the image to a bad memory location. In the best case, >> this just crashes QEMU, in the worst case, this could maybe be used to >> inject code from the kernel image into the QEMU binary, so we better fix >> it with an additional sanity check here. >> >> Cc: qemu-stable@nongnu.org >> Reported-by: Guangming Liu >> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635 >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Queued, thanks. Paolo
Hi Thomas, On 9/25/19 3:03 PM, Thomas Huth wrote: > Both, "rom->addr" and "addr" are derived from the binary image > that can be loaded with the "-kernel" paramer. The code in > rom_copy() then calculates: > > d = dest + (rom->addr - addr); > > and uses "d" as destination in a memcpy() some lines later. Now with > bad kernel images, it is possible that rom->addr is smaller than addr, > thus "rom->addr - addr" gets negative and the memcpy() then tries to > copy contents from the image to a bad memory location. In the best case, > this just crashes QEMU, in the worst case, this could maybe be used to > inject code from the kernel image into the QEMU binary, so we better fix > it with an additional sanity check here. > > Cc: qemu-stable@nongnu.org > Reported-by: Guangming Liu > Buglink: https://bugs.launchpad.net/qemu/+bug/1844635 "This page does not exist, or you may not have permission to see it." This seems security related. Shouldn't we open a CVE for this? https://wiki.qemu.org/SecurityProcess#CVE_allocation Let's say I have write access to a LAN TFTP server used by some PXE bootloader where I can store my crafted nasty kernel, then I get this score: https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1 CVSS Base Score: 9.6 CVSS Temporal Score: 8.6 Which seems quite high. > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 0d60219364..5099f27dc8 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size) $ git show 235f86ef014 Date: Thu Nov 12 21:53:11 2009 +0100 This function is old and poorly documented. > if (rom->addr + rom->romsize < addr) { > continue; > } > - if (rom->addr > end) { > + if (rom->addr > end || rom->addr < addr) { Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > break; > } > >
On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote: > Hi Thomas, > > On 9/25/19 3:03 PM, Thomas Huth wrote: >> Both, "rom->addr" and "addr" are derived from the binary image >> that can be loaded with the "-kernel" paramer. The code in >> rom_copy() then calculates: >> >> d = dest + (rom->addr - addr); >> >> and uses "d" as destination in a memcpy() some lines later. Now with >> bad kernel images, it is possible that rom->addr is smaller than addr, >> thus "rom->addr - addr" gets negative and the memcpy() then tries to >> copy contents from the image to a bad memory location. In the best case, >> this just crashes QEMU, in the worst case, this could maybe be used to >> inject code from the kernel image into the QEMU binary, so we better fix >> it with an additional sanity check here. >> >> Cc: qemu-stable@nongnu.org >> Reported-by: Guangming Liu >> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635 > > "This page does not exist, or you may not have permission to see it." > > This seems security related. Shouldn't we open a CVE for this? > https://wiki.qemu.org/SecurityProcess#CVE_allocation I wrote to the security team before writing the patch, so I assume a CVE number is already on the way. I'll reply to this thread when it is available. Thomas
On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote: [...] > Let's say I have write access to a LAN TFTP server used by some PXE > bootloader where I can store my crafted nasty kernel, then I get this score: > > https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1 > > CVSS Base Score: 9.6 > CVSS Temporal Score: 8.6 > > Which seems quite high. I don't think you can trigger this bug this way. If you load your kernel via a PXE server, the ELF parsing will be done by the bootloader, won't it? I think you can only trigger this bug here if you load your kernel via the "-kernel" command line parameter of QEMU (or the generic-loader device), so it's not a real guest escape, as far as I can see. Thomas
On 9/26/19 7:58 AM, Thomas Huth wrote: > On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote: > [...] >> Let's say I have write access to a LAN TFTP server used by some PXE >> bootloader where I can store my crafted nasty kernel, then I get this score: >> >> https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1 >> >> CVSS Base Score: 9.6 >> CVSS Temporal Score: 8.6 >> >> Which seems quite high. > > I don't think you can trigger this bug this way. If you load your kernel > via a PXE server, the ELF parsing will be done by the bootloader, won't > it? I think you can only trigger this bug here if you load your kernel > via the "-kernel" command line parameter of QEMU (or the generic-loader > device), so it's not a real guest escape, as far as I can see. Ah indeed you are correct. You have to use the -kernel option to set kernel_filename. This reduce the scores: CVSS Base Score: 8.4 CVSS Temporal Score: 7.6 Exploitability Subscore: 1.7 You could load a kernel stored on a NFS server, but it is unlikely a production case :)
© 2016 - 2024 Red Hat, Inc.