[PATCH 0/2] avoid OOB access in SD card emulator

[PATCH 0/2] avoid OOB access in SD card emulator

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

* This patch series fixes an OOB access issue while performing block
  write commands in SD card emulator.

* Second patch disables the sdhci-pci device build by default.

Thank you.
--
Prasad J Pandit (2):
  sd: check bit number before setting card_status flag
  sd: disable sdhci-pci device by default

 hw/sd/Kconfig | 1 -
 hw/sd/sd.c    | 7 ++++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH 1/2] sd: check bit number before setting card_status flag

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

SD card emulator sets 'sd->card_status' while performing block
write commands. While doing so, it tests the corresponding bit
derived from 's->data_start' address. This may lead to OOB access.
Add check to avoid it.

Reported-by: Alex <alxndr@bu.edu>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 71a9af09ab..916e9fff58 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -802,7 +802,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
 
 static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
 {
-    return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
+    uint64_t bit = sd_addr_to_wpnum(addr);
+
+    if (bit < sd->wpgrps_size) {
+        return test_bit(bit, sd->wp_groups);
+    }
+    return true;
 }
 
 static void sd_lock_command(SDState *sd)
-- 
2.26.2

[PATCH 2/2] sd: disable sdhci-pci device by default

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Disable rarely used sdhci-pci device build by default.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/sd/Kconfig b/hw/sd/Kconfig
index c5e1e5581c..93dea61285 100644
--- a/hw/sd/Kconfig
+++ b/hw/sd/Kconfig
@@ -16,6 +16,5 @@ config SDHCI
 
 config SDHCI_PCI
     bool
-    default y if PCI_DEVICES
     depends on PCI
     select SDHCI
-- 
2.26.2

Re: [PATCH 2/2] sd: disable sdhci-pci device by default

[Peter Maydell]

On Wed, 20 May 2020 at 16:28, P J P <ppandit@redhat.com> wrote:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Disable rarely used sdhci-pci device build by default.
>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---

Doesn't this break existing working command lines? The
device exists, some people use it. We should treat it like
other PCI devices -- if the guest arch/machine can handle
PCI the device should be built.

There's obviously scope for being more general and allowing
some kind of "only build the subset of devices we feel
more confident abut the security of" setup (don't RH do
something like this downstream?), but upstream we don't
have a concept like that, we just build everything.

thanks
-- PMM

Re: [PATCH 2/2] sd: disable sdhci-pci device by default

[Philippe Mathieu-Daudé]

+Kevin, Paolo, Emanuele

On 5/20/20 5:39 PM, Peter Maydell wrote:
> On Wed, 20 May 2020 at 16:28, P J P <ppandit@redhat.com> wrote:
>>
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Disable rarely used sdhci-pci device build by default.
>>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
> 
> Doesn't this break existing working command lines? The
> device exists, some people use it. We should treat it like
> other PCI devices -- if the guest arch/machine can handle
> PCI the device should be built.

Prasad, I once tried to remove it, and Kevin said he was using it:

https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02765.html

   I do find qemu's PCI SDHCI support useful for testing.
   SeaBIOS can launch an OS from PCI SDHCI (qemu-system-x86_64
   -device sdhci-pci -device sd-card,drive=drive0 -drive
   id=drive0,if=none,file=dos-drivec) and linux has drivers for
   it as well.  A number of the Chromebooks ship with PCI SDHCI
   devices on them, so it's not an unheard of configuration.

> 
> There's obviously scope for being more general and allowing
> some kind of "only build the subset of devices we feel
> more confident abut the security of" setup (don't RH do
> something like this downstream?), but upstream we don't
> have a concept like that, we just build everything.

Prasad, again back at that time I tried to remove this (as the device 
appears unused) Paolo told me after asking explanation for his comment 
"PCI devices can be created with -device, they don't have to be added by
boards." [*] - I guess it was on IRC - to check commit 224d10ff5ae, this 
device was added with RH PCI ID because it was useful for testing:

static void sdhci_pci_class_init(ObjectClass *klass, void *data)
{
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

     k->init = sdhci_pci_init;
     k->exit = sdhci_pci_exit;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     ...

This device is also nicely used as example for the qgraph testing (see 
tests/test-qgraph.c added in fc281c80202).

[*] https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02819.html

Peter, indeed the Kconfig was added to allow distributions to disable 
piece of code, and we want to keep this device in mainstream QEMU.
Distributions are free to disable it setting SDHCI_PCI=n

So to this patch:

Nack.

> 
> thanks
> -- PMM
> 

Re: [PATCH 2/2] sd: disable sdhci-pci device by default

[Daniel P. Berrangé]

On Wed, May 20, 2020 at 04:39:45PM +0100, Peter Maydell wrote:
> On Wed, 20 May 2020 at 16:28, P J P <ppandit@redhat.com> wrote:
> >
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > Disable rarely used sdhci-pci device build by default.
> >
> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> 
> Doesn't this break existing working command lines? The
> device exists, some people use it. We should treat it like
> other PCI devices -- if the guest arch/machine can handle
> PCI the device should be built.
> 
> There's obviously scope for being more general and allowing
> some kind of "only build the subset of devices we feel
> more confident abut the security of" setup (don't RH do
> something like this downstream?), but upstream we don't
> have a concept like that, we just build everything.

Yeah, disabling undesired devices is really a job for downstream and Red
Hat do indeed do this in RHEL builds of QEMU.

What's missing from an upstream side I think is largely a documentation
issue. ie a way to actually tell our users the good, bad & the ugly
for QEMU features, so they can make informed decision to disable stuff
if they wish.

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 1/2] sd: check bit number before setting card_status flag

[Philippe Mathieu-Daudé]

Hi Prasad,

On 5/20/20 5:24 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> SD card emulator sets 'sd->card_status' while performing block
> write commands. While doing so, it tests the corresponding bit
> derived from 's->data_start' address. This may lead to OOB access.
> Add check to avoid it.

Ah, this is different that the one reported recently:
https://bugs.launchpad.net/qemu/+bug/1878054

Do you have a reproducer?

Is this a CVE?

> 
> Reported-by: Alex <alxndr@bu.edu>

This is not Alexander complete name.

> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/sd/sd.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 71a9af09ab..916e9fff58 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -802,7 +802,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>   
>   static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
>   {
> -    return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +    uint64_t bit = sd_addr_to_wpnum(addr);
> +
> +    if (bit < sd->wpgrps_size) {

This should never be called with a such address, so I'd simply use an 
assertion here.

The problem is earlier where the address should be validated and a 
protocol error returned.

> +        return test_bit(bit, sd->wp_groups);
> +    }
> +    return true;
>   }
>   
>   static void sd_lock_command(SDState *sd)
> 

Re: [PATCH 2/2] sd: disable sdhci-pci device by default

[P J P]

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

+-- On Wed, 20 May 2020, Philippe Mathieu-Daudé wrote --+
| Prasad, I once tried to remove it, and Kevin said he was using it:
| 
| https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02765.html
| 
|   I do find qemu's PCI SDHCI support useful for testing.
|   SeaBIOS can launch an OS from PCI SDHCI (qemu-system-x86_64
|   -device sdhci-pci -device sd-card,drive=drive0 -drive
|   id=drive0,if=none,file=dos-drivec) and linux has drivers for
|   it as well.  A number of the Chromebooks ship with PCI SDHCI
|   devices on them, so it's not an unheard of configuration.
| 
| Prasad, again back at that time I tried to remove this (as the device appears
| unused) Paolo told me after asking explanation for his comment "PCI devices
| can be created with -device, they don't have to be added by
| boards." [*] - I guess it was on IRC - to check commit 224d10ff5ae, this
| device was added with RH PCI ID because it was useful for testing:
| 
|     ...
| 
| This device is also nicely used as example for the qgraph testing (see
| tests/test-qgraph.c added in fc281c80202).
| 
| [*] https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02819.html
| 
| Peter, indeed the Kconfig was added to allow distributions to disable piece of
| code, and we want to keep this device in mainstream QEMU.
| Distributions are free to disable it setting SDHCI_PCI=n
| 
| So to this patch:
| 
| Nack.

Right, okay. (I half expected it ;)

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D