acpi: validate hotplug selector on access

[PATCH] acpi: validate hotplug selector on access

Posted by Michael S. Tsirkin 2 years, 4 months ago

When bus is looked up on a pci write, we didn't
validate that the lookup succeeded.
Fuzzers thus can trigger QEMU crash by dereferencing the NULL
bus pointer.

Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
Cc: "Igor Mammedov" <imammedo@redhat.com>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/pcihp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 30405b5113..a5e182dd3a 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -491,6 +491,9 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
         }
 
         bus = acpi_pcihp_find_hotplug_bus(s, s->hotplug_select);
+        if (!bus) {
+            break;
+        }
         QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
             Object *o = OBJECT(kid->child);
             PCIDevice *dev = PCI_DEVICE(o);
-- 
MST

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Philippe Mathieu-Daudé 2 years, 4 months ago

+Mauro & Alex

On 12/21/21 15:48, Michael S. Tsirkin wrote:
> When bus is looked up on a pci write, we didn't
> validate that the lookup succeeded.
> Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> bus pointer.
> 
> Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> Cc: "Igor Mammedov" <imammedo@redhat.com>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

It seems this problem is important enough to get a CVE assigned.

Mauro, please update us when you get the CVE number.
Michael, please amend the CVE number before committing the fix.

FWIW Paolo asked every fuzzed bug reproducer to be committed
as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
reproducer in plain C.

Regards,

Phil.

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Michael S. Tsirkin 2 years, 4 months ago

On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> +Mauro & Alex
> 
> On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > When bus is looked up on a pci write, we didn't
> > validate that the lookup succeeded.
> > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > bus pointer.
> > 
> > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> It seems this problem is important enough to get a CVE assigned.

Guest root can crash guest.
I don't see why we would assign a CVE.

> Mauro, please update us when you get the CVE number.
> Michael, please amend the CVE number before committing the fix.
> 
> FWIW Paolo asked every fuzzed bug reproducer to be committed
> as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
> reproducer in plain C.
> 
> Regards,
> 
> Phil.

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Philippe Mathieu-Daudé 2 years, 4 months ago

On Wed, Dec 22, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> > +Mauro & Alex
> >
> > On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > > When bus is looked up on a pci write, we didn't
> > > validate that the lookup succeeded.
> > > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > > bus pointer.
> > >
> > > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > It seems this problem is important enough to get a CVE assigned.
>
> Guest root can crash guest.
> I don't see why we would assign a CVE.

Well thinking about downstream distributions, if there is a CVE assigned,
it helps them to have it written in the commit. Maybe I am mistaken.

Unrelated but it seems there is a coordination problem with the
qemu-security@ list,
if this isn't a security issue, why was a CVE requested?

> > Mauro, please update us when you get the CVE number.
> > Michael, please amend the CVE number before committing the fix.
> >
> > FWIW Paolo asked every fuzzed bug reproducer to be committed
> > as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
> > reproducer in plain C.
> >
> > Regards,
> >
> > Phil.
>

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Michael S. Tsirkin 2 years, 4 months ago

On Wed, Dec 22, 2021 at 09:27:51PM +0100, Philippe Mathieu-Daudé wrote:
> On Wed, Dec 22, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> > > +Mauro & Alex
> > >
> > > On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > > > When bus is looked up on a pci write, we didn't
> > > > validate that the lookup succeeded.
> > > > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > > > bus pointer.
> > > >
> > > > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > > > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > It seems this problem is important enough to get a CVE assigned.
> >
> > Guest root can crash guest.
> > I don't see why we would assign a CVE.
> 
> Well thinking about downstream distributions, if there is a CVE assigned,
> it helps them to have it written in the commit. Maybe I am mistaken.
> 
> Unrelated but it seems there is a coordination problem with the
> qemu-security@ list,
> if this isn't a security issue, why was a CVE requested?

Right.  I don't think a priveleged user crashing VM warrants a CVE,
it can just halt a CPU or whatever. Just cancel the CVE request pls.

> > > Mauro, please update us when you get the CVE number.
> > > Michael, please amend the CVE number before committing the fix.
> > >
> > > FWIW Paolo asked every fuzzed bug reproducer to be committed
> > > as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
> > > reproducer in plain C.
> > >
> > > Regards,
> > >
> > > Phil.
> >

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Mauro Matteo Cascella 2 years, 4 months ago

Hi,

On Wed, Dec 22, 2021 at 9:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Dec 22, 2021 at 09:27:51PM +0100, Philippe Mathieu-Daudé wrote:
> > On Wed, Dec 22, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> > > > +Mauro & Alex
> > > >
> > > > On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > > > > When bus is looked up on a pci write, we didn't
> > > > > validate that the lookup succeeded.
> > > > > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > > > > bus pointer.
> > > > >
> > > > > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > > > > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > > It seems this problem is important enough to get a CVE assigned.
> > >
> > > Guest root can crash guest.
> > > I don't see why we would assign a CVE.
> >
> > Well thinking about downstream distributions, if there is a CVE assigned,
> > it helps them to have it written in the commit. Maybe I am mistaken.
> >
> > Unrelated but it seems there is a coordination problem with the
> > qemu-security@ list,
> > if this isn't a security issue, why was a CVE requested?
>
> Right.  I don't think a priveleged user crashing VM warrants a CVE,
> it can just halt a CPU or whatever. Just cancel the CVE request pls.

While I agree with you that this is kind of borderline and I expressed
similar concerns in the past, I was told that:

1) root guest users are not necessarily trustworthy (from the host perspective).
2) NULL pointer deref and similar issues caused by an
ill-handled/error condition are CVE worthy, even if triggered by root.
3) In other cases, DoS triggered by root is not a security issue
because it's an expected behavior and not an ill-handled/error
condition (think of assert failures, for example).

In other words, "ill-handled condition" is the crucial factor that
makes a bug CVE worthy or not.

+Prasad, can you shed some light on this? Is my understanding correct?

Also, please note that we regularly get CVE requests for bugs like
this and many CVEs have been assigned in the past. Of course that
doesn't mean we can't change things going forward, but I think we
should make it clear (probably here:
https://www.qemu.org/docs/master/system/security.html) that these
kinds of bugs are not eligible for CVE assignment.

> > > > Mauro, please update us when you get the CVE number.
> > > > Michael, please amend the CVE number before committing the fix.
> > > >
> > > > FWIW Paolo asked every fuzzed bug reproducer to be committed
> > > > as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
> > > > reproducer in plain C.
> > > >
> > > > Regards,
> > > >
> > > > Phil.
> > >
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Michael S. Tsirkin 2 years, 4 months ago

On Thu, Dec 23, 2021 at 10:58:14AM +0100, Mauro Matteo Cascella wrote:
> Hi,
> 
> On Wed, Dec 22, 2021 at 9:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Dec 22, 2021 at 09:27:51PM +0100, Philippe Mathieu-Daudé wrote:
> > > On Wed, Dec 22, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> > > > > +Mauro & Alex
> > > > >
> > > > > On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > > > > > When bus is looked up on a pci write, we didn't
> > > > > > validate that the lookup succeeded.
> > > > > > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > > > > > bus pointer.
> > > > > >
> > > > > > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > > > > > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > >
> > > > > It seems this problem is important enough to get a CVE assigned.
> > > >
> > > > Guest root can crash guest.
> > > > I don't see why we would assign a CVE.
> > >
> > > Well thinking about downstream distributions, if there is a CVE assigned,
> > > it helps them to have it written in the commit. Maybe I am mistaken.
> > >
> > > Unrelated but it seems there is a coordination problem with the
> > > qemu-security@ list,
> > > if this isn't a security issue, why was a CVE requested?
> >
> > Right.  I don't think a priveleged user crashing VM warrants a CVE,
> > it can just halt a CPU or whatever. Just cancel the CVE request pls.
> 
> While I agree with you that this is kind of borderline and I expressed
> similar concerns in the past, I was told that:
> 
> 1) root guest users are not necessarily trustworthy (from the host perspective).
> 2) NULL pointer deref and similar issues caused by an
> ill-handled/error condition are CVE worthy, even if triggered by root.
> 3) In other cases, DoS triggered by root is not a security issue
> because it's an expected behavior and not an ill-handled/error
> condition (think of assert failures, for example).
> 
> In other words, "ill-handled condition" is the crucial factor that
> makes a bug CVE worthy or not.

I guess the point is that a downstream might have a slightly different
code path where it would be more serious ...
OK then, not a big deal for me. So what's the CVE # then?

> +Prasad, can you shed some light on this? Is my understanding correct?
> 
> Also, please note that we regularly get CVE requests for bugs like
> this and many CVEs have been assigned in the past. Of course that
> doesn't mean we can't change things going forward, but I think we
> should make it clear (probably here:
> https://www.qemu.org/docs/master/system/security.html) that these
> kinds of bugs are not eligible for CVE assignment.


That would be good, yes.

> > > > > Mauro, please update us when you get the CVE number.
> > > > > Michael, please amend the CVE number before committing the fix.
> > > > >
> > > > > FWIW Paolo asked every fuzzed bug reproducer to be committed
> > > > > as qtest, see tests/qtest/fuzz*c. Alex has a way to generate
> > > > > reproducer in plain C.
> > > > >
> > > > > Regards,
> > > > >
> > > > > Phil.
> > > >
> >
> 
> -- 
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Mauro Matteo Cascella 2 years, 4 months ago

On Thu, Dec 23, 2021 at 2:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Dec 23, 2021 at 10:58:14AM +0100, Mauro Matteo Cascella wrote:
> > Hi,
> >
> > On Wed, Dec 22, 2021 at 9:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Dec 22, 2021 at 09:27:51PM +0100, Philippe Mathieu-Daudé wrote:
> > > > On Wed, Dec 22, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Wed, Dec 22, 2021 at 08:19:41PM +0100, Philippe Mathieu-Daudé wrote:
> > > > > > +Mauro & Alex
> > > > > >
> > > > > > On 12/21/21 15:48, Michael S. Tsirkin wrote:
> > > > > > > When bus is looked up on a pci write, we didn't
> > > > > > > validate that the lookup succeeded.
> > > > > > > Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> > > > > > > bus pointer.
> > > > > > >
> > > > > > > Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> > > > > > > Cc: "Igor Mammedov" <imammedo@redhat.com>
> > > > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > >
> > > > > > It seems this problem is important enough to get a CVE assigned.
> > > > >
> > > > > Guest root can crash guest.
> > > > > I don't see why we would assign a CVE.
> > > >
> > > > Well thinking about downstream distributions, if there is a CVE assigned,
> > > > it helps them to have it written in the commit. Maybe I am mistaken.
> > > >
> > > > Unrelated but it seems there is a coordination problem with the
> > > > qemu-security@ list,
> > > > if this isn't a security issue, why was a CVE requested?
> > >
> > > Right.  I don't think a priveleged user crashing VM warrants a CVE,
> > > it can just halt a CPU or whatever. Just cancel the CVE request pls.
> >
> > While I agree with you that this is kind of borderline and I expressed
> > similar concerns in the past, I was told that:
> >
> > 1) root guest users are not necessarily trustworthy (from the host perspective).
> > 2) NULL pointer deref and similar issues caused by an
> > ill-handled/error condition are CVE worthy, even if triggered by root.
> > 3) In other cases, DoS triggered by root is not a security issue
> > because it's an expected behavior and not an ill-handled/error
> > condition (think of assert failures, for example).
> >
> > In other words, "ill-handled condition" is the crucial factor that
> > makes a bug CVE worthy or not.
>
> I guess the point is that a downstream might have a slightly different
> code path where it would be more serious ...
> OK then, not a big deal for me. So what's the CVE # then?

CVE-2021-4158 has been assigned for this issue.

> > +Prasad, can you shed some light on this? Is my understanding correct?
> >
> > Also, please note that we regularly get CVE requests for bugs like
> > this and many CVEs have been assigned in the past. Of course that
> > doesn't mean we can't change things going forward, but I think we
> > should make it clear (probably here:
> > https://www.qemu.org/docs/master/system/security.html) that these
> > kinds of bugs are not eligible for CVE assignment.

Thanks.
-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Ani Sinha 2 years, 4 months ago


On Tue, 21 Dec 2021, Michael S. Tsirkin wrote:

> When bus is looked up on a pci write, we didn't
> validate that the lookup succeeded.
> Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> bus pointer.

Good fix. Nice catch.

>
> Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> Cc: "Igor Mammedov" <imammedo@redhat.com>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/pcihp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 30405b5113..a5e182dd3a 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -491,6 +491,9 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
>          }
>
>          bus = acpi_pcihp_find_hotplug_bus(s, s->hotplug_select);
> +        if (!bus) {
> +            break;
> +        }
>          QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
>              Object *o = OBJECT(kid->child);
>              PCIDevice *dev = PCI_DEVICE(o);
> --
> MST
>
>

Re: [PATCH] acpi: validate hotplug selector on access

Posted by Philippe Mathieu-Daudé 2 years, 4 months ago

On 12/21/21 15:48, Michael S. Tsirkin wrote:
> When bus is looked up on a pci write, we didn't
> validate that the lookup succeeded.
> Fuzzers thus can trigger QEMU crash by dereferencing the NULL
> bus pointer.
> 
> Fixes: b32bd763a1 ("pci: introduce acpi-index property for PCI device")
> Cc: "Igor Mammedov" <imammedo@redhat.com>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/770
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/acpi/pcihp.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>