[PATCH v2 2/2] vhost: release virtqueue objects in error path
From: Prasad Pandit
vhost_dev_start function does not release virtqueue objects when
event_notifier_init() function fails. Release virtqueue objects
and log a message about function failure.
Signed-off-by: Prasad Pandit
---
hw/virtio/vhost.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
v2: split a single patch into two.
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6be4a0626a..1de3029ae7 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1942,7 +1942,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev, bool vrings)
r = event_notifier_init(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier, 0);
if (r < 0) {
-return r;
+VHOST_OPS_DEBUG(r, "event_notifier_init failed");
+goto fail_vq;
}
event_notifier_test_and_clear(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
--
2.40.1
[PATCH v2 1/2] vhost: release memory_listener object in error path
From: Prasad Pandit
vhost_dev_start function does not release memory_listener object
in case of an error. This may crash the guest when vhost is unable
to set memory table:
stack trace of thread 125653:
Program terminated with signal SIGSEGV, Segmentation fault
#0 memory_listener_register (qemu-kvm + 0x6cda0f)
#1 vhost_dev_start (qemu-kvm + 0x699301)
#2 vhost_net_start (qemu-kvm + 0x45b03f)
#3 virtio_net_set_status (qemu-kvm + 0x665672)
#4 qmp_set_link (qemu-kvm + 0x548fd5)
#5 net_vhost_user_event (qemu-kvm + 0x552c45)
#6 tcp_chr_connect (qemu-kvm + 0x88d473)
#7 tcp_chr_new_client (qemu-kvm + 0x88cf83)
#8 tcp_chr_accept (qemu-kvm + 0x88b429)
#9 qio_net_listener_channel_func (qemu-kvm + 0x7ac07c)
#10 g_main_context_dispatch (libglib-2.0.so.0 + 0x54e2f)
Release memory_listener objects in the error path.
Signed-off-by: Prasad Pandit
---
hw/virtio/vhost.c | 3 +++
1 file changed, 3 insertions(+)
v2: split a single patch into two. Mention about vhost set mem table failure
resulting in guest crash.
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 23da579ce2..6be4a0626a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2004,6 +2004,9 @@ fail_vq:
}
fail_mem:
+if (vhost_dev_has_iommu(hdev)) {
+memory_listener_unregister(>iommu_listener);
+}
fail_features:
vdev->vhost_started = false;
hdev->started = false;
--
2.40.1
[PATCH v2 0/2] vhost: release memory objects in an error path
From: Prasad Pandit Hi, vhost_dev_start function does not release memory objects in case of an error. These couple of patches fix this glitch. Thank you. --- Prasad Pandit (2): vhost: release memory_listener object in error path vhost: release virtqueue objects in error path hw/virtio/vhost.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) -- 2.40.1
[PATCH v1] vhost: release memory objects in error path
From: Prasad Pandit
vhost_dev_start function does not release memory objects in case
of an error. This may crash the guest with:
stack trace of thread 125653:
Program terminated with signal SIGSEGV, Segmentation fault
#0 memory_listener_register (qemu-kvm + 0x6cda0f)
#1 vhost_dev_start (qemu-kvm + 0x699301)
#2 vhost_net_start (qemu-kvm + 0x45b03f)
#3 virtio_net_set_status (qemu-kvm + 0x665672)
#4 qmp_set_link (qemu-kvm + 0x548fd5)
#5 net_vhost_user_event (qemu-kvm + 0x552c45)
#6 tcp_chr_connect (qemu-kvm + 0x88d473)
#7 tcp_chr_new_client (qemu-kvm + 0x88cf83)
#8 tcp_chr_accept (qemu-kvm + 0x88b429)
#9 qio_net_listener_channel_func (qemu-kvm + 0x7ac07c)
#10 g_main_context_dispatch (libglib-2.0.so.0 + 0x54e2f)
Release memory_listener and virtqueue objects in the error path.
Signed-off-by: Prasad Pandit
---
hw/virtio/vhost.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
v1: log debug message when event_notifier_init fails
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 23da579ce2..1de3029ae7 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1942,7 +1942,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev, bool vrings)
r = event_notifier_init(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier, 0);
if (r < 0) {
-return r;
+VHOST_OPS_DEBUG(r, "event_notifier_init failed");
+goto fail_vq;
}
event_notifier_test_and_clear(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
@@ -2004,6 +2005,9 @@ fail_vq:
}
fail_mem:
+if (vhost_dev_has_iommu(hdev)) {
+memory_listener_unregister(>iommu_listener);
+}
fail_features:
vdev->vhost_started = false;
hdev->started = false;
--
2.40.1
[PATCH] vhost: release memory objects in error path
From: Prasad Pandit
vhost_dev_start function does not release memory objects in case
of an error. This may crash the guest with:
stack trace of thread 125653:
Program terminated with signal SIGSEGV, Segmentation fault
#0 memory_listener_register (qemu-kvm + 0x6cda0f)
#1 vhost_dev_start (qemu-kvm + 0x699301)
#2 vhost_net_start (qemu-kvm + 0x45b03f)
#3 virtio_net_set_status (qemu-kvm + 0x665672)
#4 qmp_set_link (qemu-kvm + 0x548fd5)
#5 net_vhost_user_event (qemu-kvm + 0x552c45)
#6 tcp_chr_connect (qemu-kvm + 0x88d473)
#7 tcp_chr_new_client (qemu-kvm + 0x88cf83)
#8 tcp_chr_accept (qemu-kvm + 0x88b429)
#9 qio_net_listener_channel_func (qemu-kvm + 0x7ac07c)
#10 g_main_context_dispatch (libglib-2.0.so.0 + 0x54e2f)
===
Release memory_listener and virtqueue objects in the error path.
Signed-off-by: Prasad Pandit
---
hw/virtio/vhost.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 23da579ce2..e261ae7c49 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1942,7 +1942,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev, bool vrings)
r = event_notifier_init(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier, 0);
if (r < 0) {
-return r;
+goto fail_vq;
}
event_notifier_test_and_clear(
>vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
@@ -2004,6 +2004,9 @@ fail_vq:
}
fail_mem:
+if (vhost_dev_has_iommu(hdev)) {
+memory_listener_unregister(>iommu_listener);
+}
fail_features:
vdev->vhost_started = false;
hdev->started = false;
--
2.40.1
Re: [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value
On Wednesday, 24 August, 2022, 04:46:21 pm IST, Fiona Ebner
wrote:
>Reported by one of our users running into the failing assert():
>https://forum.proxmox.com/threads/114011/#post-492916
>
>- assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
>+ assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu <= VMXNET3_MAX_MTU);
> VMW_CFPRN("MTU is %u", s->mtu);
>
* I wonder if setting s->mtu == buffer_upper_limit may lead to an out-of-bounds
access issue?
* IIUC, VMXNET3_MAX_MTU OR s->mtu does not seem to be used to allocate and/or
access packet buffer(s)
so above check might work, but still it does not seem clean, ie. it may lead
to some confusion.
* Nonetheless, Jason has acked it, so that's good.
Thank you.
---
-P J P
http://feedmug.com
Re: [PATCH] net: vmxnet3: validate configuration values during activate (CVE-2021-20203)
On Monday, 18 October, 2021, 12:20:55 pm IST, Thomas Huth
wrote:
On 30/01/2021 14.16, P J P wrote:
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index eff299f629..4a910ca971 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1420,6 +1420,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> vmxnet3_setup_rx_filtering(s);
>> /* Cache fields from shared memory */
>> s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
>> + assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
>> VMW_CFPRN("MTU is %u", s->mtu);
>>
>> s->max_rx_frags =
>> @@ -1473,6 +1474,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> /* Read rings memory locations for TX queues */
>> pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa, conf.txRingBasePA);
>> size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa, conf.txRingSize);
>> + if (size > VMXNET3_TX_RING_MAX_SIZE) {
>> + size = VMXNET3_TX_RING_MAX_SIZE;
>> + }
>>
>> vmxnet3_ring_init(d, >txq_descr[i].tx_ring, pa, size,
>> sizeof(struct Vmxnet3_TxDesc), false);
>> @@ -1483,6 +1487,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> /* TXC ring */
>> pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa,
>>conf.compRingBasePA);
>> size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa,
>>conf.compRingSize);
>> + if (size > VMXNET3_TC_RING_MAX_SIZE) {
>> + size = VMXNET3_TC_RING_MAX_SIZE;
>> + }
>> vmxnet3_ring_init(d, >txq_descr[i].comp_ring, pa, size,
>> sizeof(struct Vmxnet3_TxCompDesc), true);
>> VMXNET3_RING_DUMP(VMW_CFPRN, "TXC", i, >txq_descr[i].comp_ring);
>> @@ -1524,6 +1531,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> /* RX rings */
>> pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa,
>>conf.rxRingBasePA[j]);
>> size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa,
>>conf.rxRingSize[j]);
>> + if (size > VMXNET3_RX_RING_MAX_SIZE) {
>> + size = VMXNET3_RX_RING_MAX_SIZE;
>> + }
>> vmxnet3_ring_init(d, >rxq_descr[i].rx_ring[j], pa, size,
>> sizeof(struct Vmxnet3_RxDesc), false);
>> VMW_CFPRN("RX queue %d:%d: Base: %" PRIx64 ", Size: %d",
>> @@ -1533,6 +1543,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> /* RXC ring */
>> pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa, conf.compRingBasePA);
>> size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa, conf.compRingSize);
>> + if (size > VMXNET3_RC_RING_MAX_SIZE) {
>> + size = VMXNET3_RC_RING_MAX_SIZE;
>> + }
>> vmxnet3_ring_init(d, >rxq_descr[i].comp_ring, pa, size,
>> sizeof(struct Vmxnet3_RxCompDesc), true);
>> VMW_CFPRN("RXC queue %d: Base: %" PRIx64 ", Size: %d", i, pa, size);
>>
>>
>Ping!
>
>According to
>https://gitlab.com/qemu-project/qemu/-/issues/308#note_705736713 this is
>still an issue...
>
>Patch looks fine to me ... maybe just add some
>qemu_log_mask(LOG_GUEST_ERROR, ...) statements before correcting the values?
* Oops! Not sure how I missed it, thought it was pulled upstream.
Will send a revised patch.
Thank you.
---
- P J P
Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
On Tuesday, 14 September, 2021, 07:00:27 pm IST, P J P
wrote:
>* Thanks so much for restarting this thread. I've been at it intermittently
>last few
> months, thinking about how could we annotate the source/module objects.
>
> -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
>
>* Last time we discussed about having both compile and run time options for
>users
> to be able to select the qualified objects/backends/devices as desired.
>
>* To confirm: How/Where is the security policy defined? Is it device/module
>specific OR QEMU project wide?
>
>>> it feels like we need
>> 'secure': 'bool'
>
>* Though we started the (above [*]) discussion with '--security' option in
>mind,
> I wonder if 'secure' annotation is much specific. And if we could widen its
> scope.
>
>
>Source annotations: I've been thinking over following approaches
>===
>
>1) Segregate the QEMU sources under
>
> ../staging/ <= devel/experimental, not for production usage
> ../src/ <= good for production usage, hence security relevant
> ../deprecated/ <= Bad for production usage, not security relevant
>
> - This is similar to Linux staging drivers' tree.
> - Staging drivers are not considered for production usage and hence CVE
> allocation.
> - At build time by default we only build sources under ../src/ tree.
> - But we can still have options to build /staging/ and /deprecated/ trees.
> - It's readily understandable to end users.
>
>2) pkgconfig(1) way:
> - If we could define per device/backend a configuration (.pc) file which is
> then used
> at build/run time to decide which sources are suitable for the build or
> usage.
>
> - I'm trying to experiment with this.
>
>3) We annotate QEMU devices/backends/modules with macros which define its
>status.
> It can then be used at build/run times to decide if it's suitable for usage.
> For ex:
>
> $ cat annotsrc.h
>
> #include
>
> enum SRCSTATUS {
> DEVEL = 0,
> STAGING,
> PRODUCTION,
> DEPRECATED
> };
>
...
>
>
>* Approach 3) above is similar to the _security_policy_taint() API,
> but works at the source/object file level, rather than specific 'struct
> type' field.
>
>* Does adding a field to struct type (ex. DeviceClass) scale to all
>objects/modules/backends etc?
> Does it have any limitations to include/cover other sources/objects?
>
>* I'd really appreciate your feedback/inputs/suggestions.
Ping...!?
---
-P J P
http://feedmug.com
Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
Hello Philippe, all
>On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé
> wrote:
>On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
>> This series is experimental! The goal is to better limit the
>> boundary of what code is considerated security critical, and
>> what is less critical (but still important!).
>>
>> This approach was quickly discussed few months ago with Markus
>> then Daniel. Instead of classifying the code on a file path
>> basis (see [1]), we insert (runtime) hints into the code
>> (which survive code movement). Offending unsafe code can
>> taint the global security policy. By default this policy
>> is 'none': the current behavior. It can be changed on the
>> command line to 'warn' to display warnings, and to 'strict'
>> to prohibit QEMU running with a tainted policy.
>
* Thanks so much for restarting this thread. I've been at it intermittently
last few
months, thinking about how could we annotate the source/module objects.
-> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
* Last time we discussed about having both compile and run time options for
users
to be able to select the qualified objects/backends/devices as desired.
* To confirm: How/Where is the security policy defined? Is it device/module
specific OR QEMU project wide?
> IOW, the reporting via QAPI introspetion is much more important
> for libvirt and mgmt apps, than any custom cli arg / printfs
> at the QEMU level.
>
* True, while it makes sense to have a solution that is conversant with
the management/libvirt layers, It'll be great if we could address qemu/cli
other use cases too.
>it feels like we need
> 'secure': 'bool'
* Though we started the (above [*]) discussion with '--security' option in mind,
I wonder if 'secure' annotation is much specific. And if we could widen its
scope.
--- x ---
Source annotations: I've been thinking over following approaches
===
1) Segregate the QEMU sources under
../staging/ <= devel/experimental, not for production usage
../src/ <= good for production usage, hence security relevant
../deprecated/ <= Bad for production usage, not security relevant
- This is similar to Linux staging drivers' tree.
- Staging drivers are not considered for production usage and hence CVE
allocation.
- At build time by default we only build sources under ../src/ tree.
- But we can still have options to build /staging/ and /deprecated/ trees.
- It's readily understandable to end users.
2) pkgconfig(1) way:
- If we could define per device/backend a configuration (.pc) file which is
then used
at build/run time to decide which sources are suitable for the build or
usage.
- I'm trying to experiment with this.
3) We annotate QEMU devices/backends/modules with macros which define its
status.
It can then be used at build/run times to decide if it's suitable for usage.
For ex:
$ cat annotsrc.h
#include
enum SRCSTATUS {
DEVEL = 0,
STAGING,
PRODUCTION,
DEPRECATED
};
uint8_t get_src_status(void);
===
$ cat libx.c
#include
#define SRC_STATUS DEPRECATED
uint8_t
get_src_status(void)
{
return SRC_STATUS;
}
===
$ cat testlibx.c
#include
#include
int
main (int argc, char *argv[])
{
printf("LibX status: %d\n", get_src_status());
return 0;
}
--- x ---
* Approach 3) above is similar to the _security_policy_taint() API,
but works at the source/object file level, rather than specific 'struct type'
field.
* Does adding a field to struct type (ex. DeviceClass) scale to all
objects/modules/backends etc?
Does it have any limitations to include/cover other sources/objects?
* I'd really appreciate any feedback/inputs/suggestions you may have.
Thank you.
---
-P J P
http://feedmug.com
Re: [PATCH] hw/rdma: Fix possible mremap overflow in the pvrdma device (CVE-2021-3582)
On Wednesday, 16 June, 2021, 04:36:09 pm IST, Marcel Apfelbaum
wrote:
>From: Marcel Apfelbaum
>diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
>index f59879e257..dadab4966b 100644
>--- a/hw/rdma/vmw/pvrdma_cmd.c
>+++ b/hw/rdma/vmw/pvrdma_cmd.c
>@@ -38,6 +38,12 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t
>pdir_dma,
> return NULL;
> }
>
>+ length = ROUND_UP(length, TARGET_PAGE_SIZE);
>+ if (nchunks * TARGET_PAGE_SIZE != length) {
>+ rdma_error_report("Invalid nchunks/length (%u, %lu)", nchunks,
>length);
>+ return NULL;
>+ }
>+
> dir = rdma_pci_dma_map(pdev, pdir_dma, TARGET_PAGE_SIZE);
> if (!dir) {
> rdma_error_report("Failed to map to page directory");
>
Looks okay.
Reviewed-by: Prasad J Pandit
Thank you.
---
-P J P
http://feedmug.com
Re: [PATCH] ui/spice-display: check NULL pointer in interface_release_resource()
+-- On Thu, 20 May 2021, Mauro Matteo Cascella wrote --+
| diff --git a/ui/spice-display.c b/ui/spice-display.c
| index d22781a23d..f59c69882d 100644
| --- a/ui/spice-display.c
| +++ b/ui/spice-display.c
| @@ -561,6 +561,10 @@ static void interface_release_resource(QXLInstance *sin,
| SimpleSpiceCursor *cursor;
| QXLCommandExt *ext;
|
| +if (!rext.info) {
| +return;
| +}
| +
| ext = (void *)(intptr_t)(rext.info->id);
| switch (ext->cmd.type) {
| case QXL_CMD_DRAW:
Looks okay.
Reviewed-by: Prasad J Pandit
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
Hello John, +-- On Tue, 18 May 2021, John Snow wrote --+ | Annotated: | | # fdctrl->cur_drv starts at 0x00 | # fdctrl->dor starts at 0x0c (DMA, RESET#) | # fdctrl->dsr starts at 0x00 | | > outb 0x3f2 0x04 | fdc_ioport_write write reg 0x02 [DOR] Digital Output Register val 0x04 | DOR changed from default after SeaBIOS init from 0x0c to 0x04 | DMA GATE# (0x08) set from 1 --> 0 | DMA GATE# appears needed to coerce fdc into a "non-dma transfer". | +RESET# remains on. Needed to avoid engaging RESET routine. | | > outb 0x3f4 0x03 | fdc_ioport_write write reg 0x04 [DSR] Date Rate Select Register val 0x03 | DSR: +DRATE SEL1 | DSR: +DRATE SEL0 | Needed to prevent "data rate mismatch" error handling by write cmd. | | The next 9 bytes (all to 0x3f5) set up the write command. | | 0x25 selects the "Write (BeOS)" command. | 0x01 selects drive1. | ... | 0x01 appears to say that a sector is "1 byte", but oddly enough no other value | seems to trigger this crash. Not sure why. Recommend investigating if you have | time. Could be transfer length calculation bug. | | > outb 0x3f3 0x04 | fdc_ioport_write write reg 0x03 [TDR] Tape Drive Register val 0x04 | TDR: +BOOTSEL | This changes the meaning of cur_drv and flips selection (as far as | I can tell) back to drive0 instead of the command's programmed drive1. | | > outb 0x3f5 0x00 | fdc_ioport_write write reg 0x05 [FIFO] Data val 0x00 | write is attempted on "drv1" which due to BOOTSEL maps back to "drv0", | which is undefined. | | This should (I hope) help guide to write a more targeted patch and a good | qtest case. * Cool, thank you so much for these details John, I appreciate it. * I'll go through the 3 fdc issues we've found open and try to fix them together as one series. Thank you. -- - P J P 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v2] fdc: check null block pointer before r/w data transfer
+-- On Tue, 18 May 2021, John Snow wrote --+ | I assume it can be rolled into the most recent issue that actually grabbed | my attention: | | -> https://gitlab.com/qemu-project/qemu/-/issues/338 | | And we can credit both reporters (and Alexander) and solve all of these issues | all at once. | | I am therefore dropping this patch from my queue. Please let me know if I am | mistaken. * It should be okay to collate patches together under above gitlab issue as a series. Considering they've individual CVEs assigned, we'll still need to tag them with CVEs, so that it's easier for downstream users to pick them. Thank you. -- - P J P 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
Hello John,
+-- On Mon, 17 May 2021, John Snow wrote --+
| > /* Selected drive */
| > -fdctrl->cur_drv = value & FD_DOR_SELMASK;
| > +if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
| > +fdctrl->cur_drv = value & FD_DOR_SELMASK;
| > +}
|
| I don't think this is correct. If you look at get_cur_drv(), it uses the
| TDR_BOOTSEL bit to change the logical mappings of "drive 0" or "drive 1" to be
| reversed. You don't check that bit here, so you might be checking the wrong
| drive.
|
| Plus, the TDR bit can change later, so I think you shouldn't actually protect
| the register write like this. Just delete this bit of code. We ought to
| protect the drives when we go to use them instead of preventing the registers
| from getting "the wrong values".
* I see.
| > cur_drv = get_cur_drv(fdctrl);
| > +if (!cur_drv->blk) {
| > +FLOPPY_DPRINTF("No drive connected\n");
| > +return 0;
| > +}
|
| This seems fine ... or at least not worse than the other error handling we
| already have here. (Which seems to be ... basically, none. We just ignore the
| write and do nothing, which seems wrong. I guess it's better than a crash...
| but I don't have the time to do a proper audit of what this is SUPPOSED to do
| in this case.)
|
| > -if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| > +if (cur_drv->blk == NULL
| > +|| blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
| > fdctrl->fifo,
|
| Seems fine, but if we had a drive for the earlier check, will we really be in
| a situation where we don't have one now?
|
| Ignore the bit I sent earlier about the qtest reproducer not correlating to
| this patch -- it does, I was experiencing an unrelated crash.
* Okay.
| On 5/17/21 7:12 AM, P J P wrote:
| > Do we need a revised patch[-series] including a qtest? OR it can be done at
| > merge time?
|
| If you have the time to write a qtest reproducer, you can send it separately
| and I'll pick it up if everything looks correct.
* Yes, that seems better, I'll try to create a qtest, but it may take time.
* I'll check and revise the patch with above details asap.
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
+-- On Sat, 15 May 2021, Philippe Mathieu-Daudé wrote --+ | This patch misses the qtest companion with the reproducer | provided by Alexander. Do we need a revised patch[-series] including a qtest? OR it can be done at merge time? Thank you. -- - P J P 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
+-- On Wed, 5 May 2021, Li Qiang wrote --+ | P J P 于2021年5月5日周三 下午3:24写道： | > - vg_ctrl_response(g, cmd, , sizeof(resp)); | > + vg_ctrl_response(g, cmd, , sizeof(resp.hdr)); | > | > * While memset(3) is okay, should it also send header(hdr) size as 'resp_len'? | | I don't think so. This function also set fields other than header such | as 'resp.capset_id', 'resp.capset_max_version' and so on. But it is passing 'resp.hdr' reference as parameter and size of 'resp' as length. sizeof(struct virtio_gpu_ctrl_hdr): 24 sizeof(struct virtio_gpu_resp_capset_info): 40 It may cause OOB access. Thank you. -- - P J P 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index c669d73a1d..a16a311d80 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
| return;
| }
|
| -virgl_renderer_resource_attach_iov(att_rb.resource_id,
| +ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
| res_iovs, att_rb.nr_entries);
| +if (ret != 0) {
| +g_free(res_iovs);
| +}
| }
* Similar to earlier,
hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
'virtio_gpu_cleanup_mapping_iov'
* should it be same for vhost-user-gpu?
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| If 'virgl_cmd_get_capset' set 'max_size' to 0,
| the 'virgl_renderer_fill_caps' will write the data after the 'resp'.
| This patch avoid this by checking the returned 'max_size'.
|
| Signed-off-by: Li Qiang
| ---
| contrib/vhost-user-gpu/virgl.c | 4
| 1 file changed, 4 insertions(+)
|
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index a16a311d80..7172104b19 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -177,6 +177,10 @@ virgl_cmd_get_capset(VuGpu *g,
|
| virgl_renderer_get_cap_set(gc.capset_id, _ver,
| _size);
| +if (!max_size) {
| +cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
| +return;
| +}
| resp = g_malloc0(sizeof(*resp) + max_size);
|
| resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;
* Looks okay.
Reviewed-by: Prasad J Pandit
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index 6a332d601f..c669d73a1d 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -108,9 +108,16 @@ virgl_cmd_resource_unref(VuGpu *g,
| struct virtio_gpu_ctrl_command *cmd)
| {
| struct virtio_gpu_resource_unref unref;
| +struct iovec *res_iovs = NULL;
| +int num_iovs = 0;
|
| VUGPU_FILL_CMD(unref);
|
| +virgl_renderer_resource_detach_iov(unref.resource_id,
| + _iovs,
| + _iovs);
| +g_free(res_iovs);
| +
| virgl_renderer_resource_unref(unref.resource_id);
* Should this also call 'virtio_gpu_cleanup_mapping_iov' similar to
'hw/display/virtio-gpu-3d.c:virgl_cmd_resource_unref'?
if (res_iovs != NULL && num_iovs != 0) {
virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
}
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
+-- On Tue, 4 May 2021, Li Qiang wrote --+ | If the guest trigger following sequences, the attach_backing will be leaked: | | vg_resource_create_2d | vg_resource_attach_backing | vg_resource_unref | | This patch fix this by freeing 'res->iov' in vg_resource_destroy. | | Signed-off-by: Li Qiang | --- | contrib/vhost-user-gpu/vhost-user-gpu.c | 1 + | 1 file changed, 1 insertion(+) | | diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c | index 0437e52b64..770dfad529 100644 | --- a/contrib/vhost-user-gpu/vhost-user-gpu.c | +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c | @@ -400,6 +400,7 @@ vg_resource_destroy(VuGpu *g, | } | | vugbm_buffer_destroy(>buffer); | +g_free(res->iov); | pixman_image_unref(res->image); | QTAILQ_REMOVE(>reslist, res, next); | g_free(res); | * Looks good. Reviewed-by: Prasad J Pandit Thank you. -- - P J P 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Check whether the 'res' has already been attach_backing to avoid
| memory leak.
|
| Signed-off-by: Li Qiang
| ---
| contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +
| 1 file changed, 5 insertions(+)
|
| diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
b/contrib/vhost-user-gpu/vhost-user-gpu.c
| index b5e153d0d6..0437e52b64 100644
| --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
| +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
| @@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
| return;
| }
|
| +if (res->iov) {
| +cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
| +return;
| +}
| +
| ret = vg_create_mapping_iov(g, , cmd, >iov);
| if (ret != 0) {
| cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
* While it'll work, should the check be done by 'virtio_gpu_find_resource()'
before returning 'res' ? ie. if 'res->iov' is being used, then it's similar
case as 'illegal resource specified %d'.
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Call 'vugbm_buffer_destroy' in error path to avoid resource leak.
|
| Signed-off-by: Li Qiang
| ---
| contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
| 1 file changed, 1 insertion(+)
|
| diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
b/contrib/vhost-user-gpu/vhost-user-gpu.c
| index f73f292c9f..b5e153d0d6 100644
| --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
| +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
| @@ -349,6 +349,7 @@ vg_resource_create_2d(VuGpu *g,
| g_critical("%s: resource creation failed %d %d %d",
| __func__, c2d.resource_id, c2d.width, c2d.height);
| g_free(res);
| +vugbm_buffer_destroy(>buffer);
| cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
| return;
| }
* Looks good.
Reviewed-by: Prasad J Pandit
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Otherwise some of the 'resp' will be leaked to guest.
|
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index 9e6660c7ab..6a332d601f 100644
|
| +memset(, 0, sizeof(resp));
| if (info.capset_index == 0) {
| resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
| virgl_renderer_get_cap_set(resp.capset_id,
- vg_ctrl_response(g, cmd, , sizeof(resp));
+ vg_ctrl_response(g, cmd, , sizeof(resp.hdr));
* While memset(3) is okay, should it also send header(hdr) size as 'resp_len'?
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1909247] Re: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
On Wednesday, 17 March, 2021, 10:26:36 pm IST, Cheolwoo Myung
wrote:
> Hello PJP, Mauro
>
> Of course. you can post the details with our reproducers.
> I'm glad it helped you.
>
> Thank you.
> - Cheolwoo Myung
>
2021년 3월 17일 (수) 오후 10:30, P J P 님이 작성:
>
>On Monday, 15 March, 2021, 07:54:30 pm IST, Mauro Matteo Cascella
> wrote:
>>JFYI, CVE-2020-35506 was assigned to a very similar (if not the same)
>>issue, see https://bugs.launchpad.net/qemu/+bug/1909247.
>
> * From the QEMU command lines below they do look similar.
>
> * CVE bug above does not link to an upstream fix/patch. Maybe it's not fixed
> yet?
>
>
>On Mon, Mar 15, 2021 at 6:58 AM P J P wrote:
> >On Monday, 15 March, 2021, 11:11:14 am IST, Cheolwoo Myung
> > wrote:
> >Using hypervisor fuzzer, hyfuzz, I found a use-after-free issue in am53c974
> >emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
> >
> >
> > Using hypervisor fuzzer, hyfuzz, I found a stack buffer overflow issue in
> > am53c974 emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
> >
* I was able to reproduce these issues against the latest upstream git source
and following patch helps to fix above two issues.
===
$ git diff hw/scsi/
diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index c3d3dab05e..4a6f208069 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -98,6 +98,7 @@ static void esp_pci_handle_abort(PCIESPState *pci, uint32_t
val)
trace_esp_pci_dma_abort(val);
if (s->current_req) {
scsi_req_cancel(s->current_req);
+s->async_len = 0;
}
}
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 507ab363bc..99bee7bc66 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -564,7 +564,7 @@ static void esp_do_dma(ESPState *s)
int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
uint8_t buf[ESP_CMDFIFO_SZ];
-len = esp_get_tc(s);
+len = MIN(esp_get_tc(s), sizeof(buf));
if (s->do_cmd) {
/*
===
> >Using hypervisor fuzzer, hyfuzz, I found a heap buffer overflow issue in
> >am53c974 emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
* This heap OOB access issue seems to occur because
static void do_busid_cmd(...)
...
buf = (uint8_t *)fifo8_pop_buf(>cmdfifo, cmdlen, ); <==
'buf' points towards an end of the 32 byte buffer allocated via
static void esp_init(Object *obj)
...
fifo8_create(>cmdfifo, ESP_CMDFIFO_SZ(=32)); <==
and the OOB access could occur at numerous places, one of which is
scsi_req_new
-> scsi_req_parse_cdb
-> memcpy(cmd->buf, buf, cmd->len); <== buf=27, cmd->len=6 <= 27+6 exceeds
limit 32.
* This one is quite tricky to fix. Because 'buf[]' is accessed at various
places with hard coded index values. It's not easy to check access
against 's->cmdfifo' object.
@Cheolwoo: is it okay with you if we post above details and your reproducers on
the upstream bug
-> https://bugs.launchpad.net/qemu/+bug/1909247
It'll help to discuss/prepare a proper fix patch.
Thank you.
---
-P J P
http://feedmug.com
** Attachment added: "hw-esp-oob-issues.zip"
https://bugs.launchpad.net/qemu/+bug/1909247/+attachment/5480385/+files/hw-esp-oob-issues.zip
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1909247
Title:
QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
Status in QEMU:
New
Bug description:
A use-after-free vulnerability was found in the am53c974 SCSI host bus
adapter emulation of QEMU. It could occur in the esp_do_dma() function
in hw/scsi/esp.c while handling the 'Information Transfer' command
(CMD_TI). A privileged guest user may abuse this flaw to crash the
QEMU process on the host, resulting in a denial of service or
potential code execution with
Re: [QEMU-SECURITY] [PATCH V4 00/10] Detect reentrant RX casued by loopback
Hello all, Just to note: * Let's use list to review non-public/embargoed patch(es) only. * If patch(es) is being reviewed publicly on list, CC'ing list does not help much. Thank you. --- -P J P http://feedmug.com
Re: [PATCH V2 7/7] rtl8193: switch to use qemu_receive_packet() for loopback
+-- On Tue, 2 Mar 2021, Jason Wang wrote --+
| DPRINTF("+++ transmit loopback mode\n");
| -rtl8139_do_receive(qemu_get_queue(s->nic), buf, size, do_interrupt);
| +qemu_receive_packet(qemu_get_queue(s->nic), buf, size);
|
...
|[PATCH V2 7/7] rtl8193: switch to use qemu_receive_packet() for loopback
* Patch 'V2' need not be here.
Thank you.
--
- P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [QEMU-SECURITY] [PATCH 1/6] net: introduce qemu_receive_packet()
Hello Alex, On Thursday, 25 February, 2021, 10:00:33 pm IST, Alexander Bulekov wrote: On 210225 1128, Alexander Bulekov wrote: > On 210225 1931, P J P wrote: > > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ > > | On 2/24/21 2:17 PM, Jason Wang wrote: > > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > > | >> IIUC the guest could trigger an infinite loop and brick the emulated > > | >> device model. Likely exhausting the stack, so either SEGV by > > corruption > > | >> or some ENOMEM? > > | > > > | > Yes. > > | >> > > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ > > list > > | >> and ask for a CVE for this issue, so distributions can track the > > patches > > | >> to backport in their stable releases? (it seems to be within the KVM > > | >> devices boundary). > > | > > > | > > > | > That's the plan. I discussed this with Prasad before and he promise to > > | > ask CVE for this. > > > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. > > What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't > those just manifestations of this bug for the e1000 and the eepro100 > devices * You mean manifestations of the dam re-entrancy issue? * They have separate CVEs because they are fixed individually. Thank you. --- -P J P http://feedmug.com
Re: [PATCH 1/6] net: introduce qemu_receive_packet()
+-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ | On 2/24/21 2:17 PM, Jason Wang wrote: | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: | >> IIUC the guest could trigger an infinite loop and brick the emulated | >> device model. Likely exhausting the stack, so either SEGV by corruption | >> or some ENOMEM? | > | > Yes. | >> | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list | >> and ask for a CVE for this issue, so distributions can track the patches | >> to backport in their stable releases? (it seems to be within the KVM | >> devices boundary). | > | > | > That's the plan. I discussed this with Prasad before and he promise to | > ask CVE for this. 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] net: eepro100: validate various address values
Hello Stefan,
+-- On Fri, 19 Feb 2021, Stefan Weil wrote --+
| If there are no recursions in normal use, the following patch should work:
|
| diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
| index 16e95ef9cc..2474cf3dc2 100644
| --- a/hw/net/eepro100.c
| +++ b/hw/net/eepro100.c
| @@ -279,6 +279,9 @@ typedef struct {
| /* Quasi static device properties (no need to save them). */
| uint16_t stats_size;
| bool has_extended_tcb_support;
| +
| + /* Flag to avoid recursions. */
| + bool busy;
| } EEPRO100State;
|
| /* Word indices in EEPROM. */
| @@ -837,6 +840,14 @@ static void action_command(EEPRO100State *s)
| Therefore we limit the number of iterations. */
| unsigned max_loop_count = 16;
|
| + if (s->busy) {
| + /* Prevent recursions. */
| + logout("recursion in %s:%u\n", __FILE__, __LINE__);
| + return;
| + }
| +
| + s->busy = true;
| +
| for (;;) {
| bool bit_el;
| bool bit_s;
| @@ -933,6 +944,7 @@ static void action_command(EEPRO100State *s)
| }
| TRACE(OTHER, logout("CU list empty\n"));
| /* List is empty. Now CU is idle or suspended. */
| + s->busy = false;
| }
|
| static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
Please see:
->
https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Feepro100_stackoverflow1
* It does not seem to address above case.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] net: e1000: check transmit descriptor field values
+-- On Thu, 18 Feb 2021, Alexander Bulekov wrote --+ | Is this a DMA re-entracy/stack-overflow issue? IIRC the plan was to have | some sort of wider fix for these issues. There are bunch of these reports | floating around at this point, I believe. * It is similar to the eepro100 one. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] net: eepro100: validate various address values
Hello Alex, Stefan, all
+-- On Thu, 18 Feb 2021, Alexander Bulekov wrote --+
| Maybe the infinite loop mentioned in the commit message is actually a DMA
| recursion issue? I'm providing a reproducer for a DMA re-entracy issue
| below. With this patch applied, the reproducer triggers the assert(), rather
| than overflowing the stack, so maybe it is the same issue? -Alex
|
| cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
| 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \
| -qtest stdio
| outl 0xcf8 0x80001014
| outl 0xcfc 0xc000
| outl 0xcf8 0x80001010
| outl 0xcfc 0xe002
| outl 0xcf8 0x80001004
| outw 0xcfc 0x7
| write 0x1c0b 0x1 0x55
| write 0x1c0c 0x1 0xfc
| write 0x1c0d 0x1 0x46
| write 0x1c0e 0x1 0x07
| write 0x746fc59 0x1 0x02
| write 0x746fc5b 0x1 0x02
| write 0x746fc5c 0x1 0xe0
| write 0x4 0x1 0x07
| write 0x5 0x1 0xfc
| write 0x6 0x1 0xff
| write 0x7 0x1 0x1f
| outw 0xc002 0x20
| EOF
|
* Yes, it is an infinite recursion induced stack overflow. I should've said
recursion instead of loop.
Thank you for sharing a reproducer and the stack trace.
+-- On Thu, 18 Feb 2021, Stefan Weil wrote --+
| Am 18.02.21 um 15:41 schrieb Peter Maydell:
|| +assert (s->cb_address >= s->cu_base);
| > We get these values from the guest; you can't just assert() on them. You
| > need to do something else.
| >
http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf
|
| I agree with Peter. The hardware emulation in QEMU should try to do the same
| as the real hardware.
* Agreed.
* While the manual does not say how to handle uint32_t overflow in above
'cb_address' calculation, I doubt if overflow is expected.
+if (s->cb_address < s->cu_base) {
+logout ("invalid cb_address: %s: %u\n", __func__, s->cb_address);
+break;
+}
I tried above fix first, it does not seem to arrest the recurssion induced
stack overflow. Hence assert(3).
* I also tried to find out if there's any cap on the 'cu_offset' value OR
number of command units (cu) that can be addressed.
But in linear addressing mode offset is a 32bit value with base address set
to zero(0).
And in 32bit segmented addressing mode 'offset' is 16bit value with
non-zero(0) base address. ie. maximum offset could be about ~4K for 16byte
command block IIUC. I'm not sure if segmented addressing mode is supported.
* I'd appreciate if you could suggest a right way to fix it OR propose/post
another patch. I'm okay either way.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH] net: eepro100: validate various address values
From: Prasad J Pandit
While processing controller commands, eepro100 emulator gets
command unit(CU) base address OR receive unit (RU) base address
OR command block (CB) address from guest. If these values are not
checked, it may lead to an infinite loop kind of issues. Add checks
to avoid it.
Reported-by: Ruhr-University Bochum
Signed-off-by: Prasad J Pandit
---
hw/net/eepro100.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 16e95ef9cc..afa1c9b2aa 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s)
bool bit_i;
bool bit_nc;
uint16_t ok_status = STATUS_OK;
-s->cb_address = s->cu_base + s->cu_offset;
+s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */
+assert (s->cb_address >= s->cu_base);
read_cb(s);
bit_el = ((s->tx.command & COMMAND_EL) != 0);
bit_s = ((s->tx.command & COMMAND_S) != 0);
@@ -860,6 +861,7 @@ static void action_command(EEPRO100State *s)
}
s->cu_offset = s->tx.link;
+assert(s->cu_offset > 0);
TRACE(OTHER,
logout("val=(cu start), status=0x%04x, command=0x%04x,
link=0x%08x\n",
s->tx.status, s->tx.command, s->tx.link));
@@ -990,8 +992,10 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t
val)
break;
case CU_CMD_BASE:
/* Load CU base. */
+assert(get_cu_state(s) == cu_idle);
TRACE(OTHER, logout("val=0x%02x (CU base address)\n", val));
s->cu_base = e100_read_reg4(s, SCBPointer);
+assert(!s->cu_base);
break;
case CU_DUMPSTATS:
/* Dump and reset statistical counters. */
@@ -1048,8 +1052,10 @@ static void eepro100_ru_command(EEPRO100State * s,
uint8_t val)
break;
case RX_ADDR_LOAD:
/* Load RU base. */
+assert(get_ru_state(s) == ru_idle);
TRACE(OTHER, logout("val=0x%02x (RU base address)\n", val));
s->ru_base = e100_read_reg4(s, SCBPointer);
+assert(!s->ru_base);
break;
default:
logout("val=0x%02x (undefined RU command)\n", val);
--
2.29.2
Re: [PATCH] net: e1000: check transmit descriptor field values
Hello Jason, +-- On Thu, 18 Feb 2021, Jason Wang wrote --+ | On 2021/2/10 下午10:52, P J P wrote: | > From: Prasad J Pandit | > | > While processing transmit (tx) descriptors in process_tx_desc() | > various descriptor fields are not checked properly. This may lead | > to infinite loop like issue. Add checks to avoid them. | > | > Reported-by: Alexander Bulekov | > Reported-by: Cheolwoo Myung | > Reported-by: Ruhr-University Bochum | > Signed-off-by: Prasad J Pandit | > --- | > hw/net/e1000.c | 6 ++ | > 1 file changed, 6 insertions(+) | | I guess you post the wrong patch :) ? Wrong patch...? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH] net: e1000: check transmit descriptor field values
From: Prasad J Pandit
While processing transmit (tx) descriptors in process_tx_desc()
various descriptor fields are not checked properly. This may lead
to infinite loop like issue. Add checks to avoid them.
Reported-by: Alexander Bulekov
Reported-by: Cheolwoo Myung
Reported-by: Ruhr-University Bochum
Signed-off-by: Prasad J Pandit
---
hw/net/e1000.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d8da2f6528..15949a3d64 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -667,9 +667,11 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
addr = le64_to_cpu(dp->buffer_addr);
if (tp->cptse) {
+assert(tp->tso_props.hdr_len);
msh = tp->tso_props.hdr_len + tp->tso_props.mss;
do {
bytes = split_size;
+assert(msh > tp->size);
if (tp->size + bytes > msh)
bytes = msh - tp->size;
@@ -681,22 +683,26 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
memmove(tp->header, tp->data, tp->tso_props.hdr_len);
}
tp->size = sz;
+assert(tp->size); /* sz may get truncated */
addr += bytes;
if (sz == msh) {
xmit_seg(s);
memmove(tp->data, tp->header, tp->tso_props.hdr_len);
tp->size = tp->tso_props.hdr_len;
}
+assert(split_size >= bytes);
split_size -= bytes;
} while (bytes && split_size);
} else {
split_size = MIN(sizeof(tp->data) - tp->size, split_size);
+assert(tp->size && split_size);
pci_dma_read(d, addr, tp->data + tp->size, split_size);
tp->size += split_size;
}
if (!(txd_lower & E1000_TXD_CMD_EOP))
return;
+assert(tp->size && tp->tso_props.hdr_len);
if (!(tp->cptse && tp->size < tp->tso_props.hdr_len)) {
xmit_seg(s);
}
--
2.29.2
Re: [QEMU-SECURITY] [PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
On Tuesday, 2 February, 2021, 08:45:19 pm IST, Peter Maydell wrote: >On the CVE: > >Since this can affect systems using KVM, this is a security bug for >us. However, it only affects an uncommon configuration: >you are only vulnerable if you are using "kernel-irqchip=off" >(the default is 'on', and turning it off is an odd thing to do). > 'CVE-2021-20221' assigned by Red Hat Inc. -> https://bugs.launchpad.net/qemu/+bug/1914353/comments/3 Thank you. --- -P J P http://feedmug.com
Re: [PULL 00/21] target-arm queue
+-- On Wed, 3 Feb 2021, Philippe Mathieu-Daudé wrote --+ | FYI Prasad mentioned a CVE was requested: | https://www.mail-archive.com/qemu-devel@nongnu.org/msg778659.html | | As you said it is an odd configuration, I am not sure it is worth | to wait for the CVE number to add it to the commit (which helps | downstream distributions tracking these). | | [updating] | | Just got detail from Prasad on IRC, it usually takes ~1 day to get | the CVE number assigned, so maybe worth postponing this until tomorrow. | | Prasad, can you reply to this message ASAP once you get the number? 'CVE-2021-20221' assigned by Red Hat Inc. -> https://bugs.launchpad.net/qemu/+bug/1914353/comments/3 Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1914353] Re: QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
'CVE-2021-20221' assigned by Red Hat Inc.
** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2021-20221
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1914353
Title:
QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
Status in QEMU:
New
Bug description:
Via [qemu-security] list
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| >- Table 4-21 GICD_SGIR bit assignments
| >
| >The Interrupt ID of the SGI to forward to the specified CPU
| >interfaces. The value of this field is the Interrupt ID, in
| >the range 0-15, for example a value of 0b0011 specifies
| >Interrupt ID 3.
| >
...
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
...
On 210202 1221, Peter Maydell wrote:
> In both cases the overrun is on the first writel to 0x8000f00,
> but the fuzzer has for some reason not reported that but instead
> blundered on until it happens to trigger some other issue that
> resulted from the memory corruption it induced with the first write.
>
...
> On the CVE:
>
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
>
> thanks
> -- PMM
>
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1914353/+subscriptions
[Bug 1914353] Re: QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
CVE requested.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1914353
Title:
QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
Status in QEMU:
New
Bug description:
Via [qemu-security] list
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| >- Table 4-21 GICD_SGIR bit assignments
| >
| >The Interrupt ID of the SGI to forward to the specified CPU
| >interfaces. The value of this field is the Interrupt ID, in
| >the range 0-15, for example a value of 0b0011 specifies
| >Interrupt ID 3.
| >
...
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
...
On 210202 1221, Peter Maydell wrote:
> In both cases the overrun is on the first writel to 0x8000f00,
> but the fuzzer has for some reason not reported that but instead
> blundered on until it happens to trigger some other issue that
> resulted from the memory corruption it induced with the first write.
>
...
> On the CVE:
>
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
>
> thanks
> -- PMM
>
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1914353/+subscriptions
[Bug 1914353] [NEW] QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
*** This bug is a security vulnerability ***
Public security bug reported:
Via [qemu-security] list
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| >- Table 4-21 GICD_SGIR bit assignments
| >
| >The Interrupt ID of the SGI to forward to the specified CPU
| >interfaces. The value of this field is the Interrupt ID, in
| >the range 0-15, for example a value of 0b0011 specifies
| >Interrupt ID 3.
| >
...
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
...
On 210202 1221, Peter Maydell wrote:
> In both cases the overrun is on the first writel to 0x8000f00,
> but the fuzzer has for some reason not reported that but instead
> blundered on until it happens to trigger some other issue that
> resulted from the memory corruption it induced with the first write.
>
...
> On the CVE:
>
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
>
> thanks
> -- PMM
>
** Affects: qemu
Importance: Undecided
Status: New
** Tags: cve security
** Information type changed from Private Security to Public Security
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1914353
Title:
QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
Status in QEMU:
New
Bug description:
Via [qemu-security] list
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| >- Table 4-21 GICD_SGIR bit assignments
| >
| >The Interrupt ID of the SGI to forward to the specified CPU
| >interfaces. The value of this field is the Interrupt ID, in
| >the range 0-15, for example a value of 0b0011 specifies
| >Interrupt ID 3.
| >
...
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
...
On 210202 1221, Peter Maydell wrote:
> In both cases the overrun is on the first writel to 0x8000f00,
> but the fuzzer has for some reason not reported that but instead
> blundered on until it happens to trigger some other issue that
> resulted from the memory corruption it induced with the first write.
>
...
> On the CVE:
>
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
>
> thanks
> -- PMM
>
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1914353/+subscriptions
[Bug 1914353] Re: QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
Upstream patch:
-> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00709.html
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1914353
Title:
QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID
Status in QEMU:
New
Bug description:
Via [qemu-security] list
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| >- Table 4-21 GICD_SGIR bit assignments
| >
| >The Interrupt ID of the SGI to forward to the specified CPU
| >interfaces. The value of this field is the Interrupt ID, in
| >the range 0-15, for example a value of 0b0011 specifies
| >Interrupt ID 3.
| >
...
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
...
On 210202 1221, Peter Maydell wrote:
> In both cases the overrun is on the first writel to 0x8000f00,
> but the fuzzer has for some reason not reported that but instead
> blundered on until it happens to trigger some other issue that
> resulted from the memory corruption it induced with the first write.
>
...
> On the CVE:
>
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
>
> thanks
> -- PMM
>
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1914353/+subscriptions
[Bug 1914236] Re: QEMU: scsi: use-after-free in mptsas_process_scsi_io_request() of mptsas1068 emulator
Upstream patch -> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00488.html -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1914236 Title: QEMU: scsi: use-after-free in mptsas_process_scsi_io_request() of mptsas1068 emulator Status in QEMU: New Bug description: * Cheolwoo Myung of Seoul National University reported a use-after-free issue in the SCSI Megaraid emulator of the QEMU. * It occurs while handling mptsas_process_scsi_io_request(), as it does not check a list in s->pending. * This was found in version 5.2.0 (master) ==31872==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000107568 at pc 0x564514950c7c bp 0x7fff524ef4b0 sp 0x7fff524ef4a0 WRITE of size 8 at 0x60c000107568 thread T0 #0 0x564514950c7b in mptsas_process_scsi_io_request ../hw/scsi/mptsas.c:306 #1 0x564514950c7b in mptsas_fetch_request ../hw/scsi/mptsas.c:775 #2 0x564514950c7b in mptsas_fetch_requests ../hw/scsi/mptsas.c:790 #3 0x56451585c25d in aio_bh_poll ../util/async.c:164 #4 0x5645158d7e7d in aio_dispatch ../util/aio-posix.c:381 #5 0x56451585be2d in aio_ctx_dispatch ../util/async.c:306 #6 0x7f1cc8af4416 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c416) #7 0x56451583f059 in glib_pollfds_poll ../util/main-loop.c:221 #8 0x56451583f059 in os_host_main_loop_wait ../util/main-loop.c:244 #9 0x56451583f059 in main_loop_wait ../util/main-loop.c:520 #10 0x56451536b181 in qemu_main_loop ../softmmu/vl.c:1537 #11 0x5645143ddd3d in main ../softmmu/main.c:50 #12 0x7f1cc2650b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) #13 0x5645143eece9 in _start (/home/cwmyung/prj/hyfuzz/src/qemu-repro/build/qemu-system-i386+0x1d55ce9) 0x60c000107568 is located 104 bytes inside of 120-byte region [0x60c000107500,0x60c000107578) freed by thread T0 here: #0 0x7f1cca9777a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8) #1 0x56451495008b in mptsas_process_scsi_io_request ../hw/scsi/mptsas.c:358 #2 0x56451495008b in mptsas_fetch_request ../hw/scsi/mptsas.c:775 #3 0x56451495008b in mptsas_fetch_requests ../hw/scsi/mptsas.c:790 #4 0x7fff524ef8bf () previously allocated by thread T0 here: #0 0x7f1cca977d28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28) #1 0x7f1cc8af9b10 in g_malloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10) #2 0x7fff524ef8bf () SUMMARY: AddressSanitizer: heap-use-after-free ../hw/scsi/mptsas.c:306 in mptsas_process_scsi_io_request Shadow bytes around the buggy address: 0x0c1880018e50: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c1880018e60: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa 0x0c1880018e70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c1880018e80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c1880018e90: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa =>0x0c1880018ea0: fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fa 0x0c1880018eb0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c1880018ec0: 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa fa 0x0c1880018ed0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c1880018ee0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c1880018ef0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==31872==ABORTING To reproduce this issue, please run the QEMU with the following command line. # To enable ASan option, please set configuration with the following command $ ./configure --target-list=i386-softmmu --disable-werror --enable-sanitizers $ make # To reproduce this issue, please run the QEMU process with the following command line. $ ./qemu-system-i386 -m 512 -drive file=./hyfuzz.img,index=0,media=disk,format=raw -device mptsas1068,id=scsi -device scsi-hd,drive=SysDisk -drive id=SysDisk,if=none,file=./disk.img To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1914236/+subscriptions
[Bug 1914236] Re: QEMU: scsi: use-after-free in mptsas_process_scsi_io_request() of mptsas1068 emulator
** Information type changed from Private Security to Public Security -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1914236 Title: QEMU: scsi: use-after-free in mptsas_process_scsi_io_request() of mptsas1068 emulator Status in QEMU: New Bug description: * Cheolwoo Myung of Seoul National University reported a use-after-free issue in the SCSI Megaraid emulator of the QEMU. * It occurs while handling mptsas_process_scsi_io_request(), as it does not check a list in s->pending. * This was found in version 5.2.0 (master) ==31872==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000107568 at pc 0x564514950c7c bp 0x7fff524ef4b0 sp 0x7fff524ef4a0 WRITE of size 8 at 0x60c000107568 thread T0 #0 0x564514950c7b in mptsas_process_scsi_io_request ../hw/scsi/mptsas.c:306 #1 0x564514950c7b in mptsas_fetch_request ../hw/scsi/mptsas.c:775 #2 0x564514950c7b in mptsas_fetch_requests ../hw/scsi/mptsas.c:790 #3 0x56451585c25d in aio_bh_poll ../util/async.c:164 #4 0x5645158d7e7d in aio_dispatch ../util/aio-posix.c:381 #5 0x56451585be2d in aio_ctx_dispatch ../util/async.c:306 #6 0x7f1cc8af4416 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c416) #7 0x56451583f059 in glib_pollfds_poll ../util/main-loop.c:221 #8 0x56451583f059 in os_host_main_loop_wait ../util/main-loop.c:244 #9 0x56451583f059 in main_loop_wait ../util/main-loop.c:520 #10 0x56451536b181 in qemu_main_loop ../softmmu/vl.c:1537 #11 0x5645143ddd3d in main ../softmmu/main.c:50 #12 0x7f1cc2650b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) #13 0x5645143eece9 in _start (/home/cwmyung/prj/hyfuzz/src/qemu-repro/build/qemu-system-i386+0x1d55ce9) 0x60c000107568 is located 104 bytes inside of 120-byte region [0x60c000107500,0x60c000107578) freed by thread T0 here: #0 0x7f1cca9777a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8) #1 0x56451495008b in mptsas_process_scsi_io_request ../hw/scsi/mptsas.c:358 #2 0x56451495008b in mptsas_fetch_request ../hw/scsi/mptsas.c:775 #3 0x56451495008b in mptsas_fetch_requests ../hw/scsi/mptsas.c:790 #4 0x7fff524ef8bf () previously allocated by thread T0 here: #0 0x7f1cca977d28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28) #1 0x7f1cc8af9b10 in g_malloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10) #2 0x7fff524ef8bf () SUMMARY: AddressSanitizer: heap-use-after-free ../hw/scsi/mptsas.c:306 in mptsas_process_scsi_io_request Shadow bytes around the buggy address: 0x0c1880018e50: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c1880018e60: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa 0x0c1880018e70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c1880018e80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c1880018e90: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa =>0x0c1880018ea0: fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fa 0x0c1880018eb0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c1880018ec0: 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa fa 0x0c1880018ed0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c1880018ee0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c1880018ef0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==31872==ABORTING To reproduce this issue, please run the QEMU with the following command line. # To enable ASan option, please set configuration with the following command $ ./configure --target-list=i386-softmmu --disable-werror --enable-sanitizers $ make # To reproduce this issue, please run the QEMU process with the following command line. $ ./qemu-system-i386 -m 512 -drive file=./hyfuzz.img,index=0,media=disk,format=raw -device mptsas1068,id=scsi -device scsi-hd,drive=SysDisk -drive id=SysDisk,if=none,file=./disk.img To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1914236/+subscriptions
[PATCH] scsi: mptsas: dequeue request object in case of an error (CVE-2021-3392)
From: Prasad J Pandit While processing SCSI i/o requests in mptsas_process_scsi_io_request(), the Megaraid emulator appends new MPTSASRequest object 'req' to the 's->pending' queue. In case of an error, this same object gets dequeued in mptsas_free_request() only if SCSIRequest object 'req->sreq' is initialised. This may lead to a use-after-free issue. Unconditionally dequeue 'req' object from 's->pending' to avoid it. Fixes: CVE-2021-3392 Buglink: https://bugs.launchpad.net/qemu/+bug/1914236 Reported-by: Cheolwoo Myung Signed-off-by: Prasad J Pandit --- hw/scsi/mptsas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index f86616544b..adff5b0bf2 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -257,8 +257,8 @@ static void mptsas_free_request(MPTSASRequest *req) req->sreq->hba_private = NULL; scsi_req_unref(req->sreq); req->sreq = NULL; -QTAILQ_REMOVE(>pending, req, next); } +QTAILQ_REMOVE(>pending, req, next); qemu_sglist_destroy(>qsg); g_free(req); } -- 2.29.2
Re: [QEMU-SECURITY] [PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
On Sunday, 31 January, 2021, 08:48:26 pm IST, Philippe Mathieu-Daudé
wrote:
>Forwarding to qemu-security@ to see if this issue is worth a CVE.
>
> | On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
> | > Per the ARM Generic Interrupt Controller Architecture specification
> | > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
> | > not 10:
> | >
> | > - Table 4-21 GICD_SGIR bit assignments
> | >
> | > The Interrupt ID of the SGI to forward to the specified CPU
> | > interfaces. The value of this field is the Interrupt ID, in
> | > the range 0-15, for example a value of 0b0011 specifies
> | > Interrupt ID 3.
> | >
> | > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> | > index af41e2fb448..75316329516 100644
> | > --- a/hw/intc/arm_gic.c
> | > +++ b/hw/intc/arm_gic.c
> | > @@ -1476,7 +1476,7 @@ static void gic_dist_writel(void *opaque, hwaddr
> offset,
> | > int target_cpu;
> | >
> | > cpu = gic_get_current_cpu(s);
> | > - irq = value & 0x3ff;
> | > + irq = value & 0xf;
> | > switch ((value >> 24) & 3) {
> | > case 0:
> | > mask = (value >> 16) & ALL_CPU_MASK;
> | >
> | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
> | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
* Does above patch address both these bugs? For BZ#1913917 'irq' is derived
from 'offset' it seems.
/* Interrupt Configuration. */
irq = (offset - 0xc00) * 4;
> | > Correct the irq mask to fix an undefined behavior (which eventually
> | > lead to a heap-buffer-overflow, see [Buglink]):
> | >
> | > $ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
> virt,accel=qtest -qtest stdio
> | > [I 1612088147.116987] OPENED
> | > [R +0.278293] writel 0x8000f00 0xff4affb0
> | > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
> type 'uint8_t [16][8]'
> | > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> ../hw/intc/arm_gic.c:1498:13
> | >
> | > Cc: qemu-sta...@nongnu.org
> | > Fixes: 9ee6e8bb853 ("ARMv7 support.")
> |
> | > ---
> | > Isnt it worth a CVE to help distributions track backports?
> | > ---
Thank you for reporting this issue. Will process further.
Thank you.
---
-P J P
http://feedmug.com
Re: [PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
| > Per the ARM Generic Interrupt Controller Architecture specification
| > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
| > not 10:
| >
| > - Table 4-21 GICD_SGIR bit assignments
| >
| > The Interrupt ID of the SGI to forward to the specified CPU
| > interfaces. The value of this field is the Interrupt ID, in
| > the range 0-15, for example a value of 0b0011 specifies
| > Interrupt ID 3.
| >
| > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
| > index af41e2fb448..75316329516 100644
| > --- a/hw/intc/arm_gic.c
| > +++ b/hw/intc/arm_gic.c
| > @@ -1476,7 +1476,7 @@ static void gic_dist_writel(void *opaque, hwaddr
offset,
| > int target_cpu;
| >
| > cpu = gic_get_current_cpu(s);
| > -irq = value & 0x3ff;
| > +irq = value & 0xf;
| > switch ((value >> 24) & 3) {
| > case 0:
| > mask = (value >> 16) & ALL_CPU_MASK;
| >
* Looks okay.
| > Correct the irq mask to fix an undefined behavior (which eventually
| > lead to a heap-buffer-overflow, see [Buglink]):
| >
| >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M
virt,accel=qtest -qtest stdio
| >[I 1612088147.116987] OPENED
| > [R +0.278293] writel 0x8000f00 0xff4affb0
| > ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for
type 'uint8_t [16][8]'
| > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../hw/intc/arm_gic.c:1498:13
| >
| > Cc: qemu-sta...@nongnu.org
| > Fixes: 9ee6e8bb853 ("ARMv7 support.")
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
|
| > ---
| > Isnt it worth a CVE to help distributions track backports?
| > ---
* Please send such report(s) to 'qemu-security' list to be triaged as
potential security ones.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1890152] Re: malloc 0xff0000030 bytes with vmxnet3
*** This bug is a duplicate of bug 1913873 *** https://bugs.launchpad.net/bugs/1913873 ** This bug has been marked a duplicate of bug 1913873 QEMU: net: vmxnet: integer overflow may crash guest -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1890152 Title: malloc 0xff030 bytes with vmxnet3 Status in QEMU: New Bug description: Hello, This reproducer causes vmxnet3 to malloc 0xff030 bytes cat << EOF | ./i386-softmmu/qemu-system-i386 \ -device vmxnet3 -m 64 -nodefaults -qtest stdio -nographic outl 0xcf8 0x80001014 outl 0xcfc 0xe0001000 outl 0xcf8 0x80001018 outl 0xcf8 0x80001004 outw 0xcfc 0x7 write 0x0 0x1 0xe1 write 0x1 0x1 0xfe write 0x2 0x1 0xbe write 0x3 0x1 0xba write 0x3e 0x1 0x05 write 0x28 0x1 0xe1 write 0x29 0x1 0xfe write 0x2a 0x1 0xff write 0x2b 0x1 0xff write 0x2c 0x1 0xff write 0x2d 0x1 0xff write 0x2e 0x1 0xff write 0x2f 0x1 0xff write 0x31c 0x1 0xff writeq 0xe0001020 0xef0bff5ecafe EOF = ==25727==ERROR: AddressSanitizer: allocator is out of memory trying to allocate 0xff030 bytes #0 0x56476a43731d in malloc (/home/alxndr/Development/qemu/general-fuzz/build/i386-softmmu/qemu-system-i386+0x2bba31d) #1 0x7fca345a8500 in g_malloc (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x54500) #2 0x56476c616312 in vmxnet3_activate_device /home/alxndr/Development/qemu/general-fuzz/hw/net/vmxnet3.c:1504:5 #3 0x56476c6101ba in vmxnet3_handle_command /home/alxndr/Development/qemu/general-fuzz/hw/net/vmxnet3.c:1576:9 #4 0x56476c60d30f in vmxnet3_io_bar1_write /home/alxndr/Development/qemu/general-fuzz/hw/net/vmxnet3.c:1772:9 #5 0x56476b11d383 in memory_region_write_accessor /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:483:5 #6 0x56476b11c827 in access_with_adjusted_size /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18 #7 0x56476b11a446 in memory_region_dispatch_write /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1466:16 #8 0x56476a4cb696 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23 #9 0x56476a4b3eb6 in flatview_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14 #10 0x56476a4b39d7 in address_space_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18 #11 0x56476b1c4614 in qtest_process_command /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:452:13 #12 0x56476b1bbc18 in qtest_process_inbuf /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:710:9 #13 0x56476b1ba8a5 in qtest_read /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:722:5 #14 0x56476e063f03 in qemu_chr_be_write_impl /home/alxndr/Development/qemu/general-fuzz/chardev/char.c:188:9 #15 0x56476e064087 in qemu_chr_be_write /home/alxndr/Development/qemu/general-fuzz/chardev/char.c:200:9 #16 0x56476e078373 in fd_chr_read /home/alxndr/Development/qemu/general-fuzz/chardev/char-fd.c:68:9 #17 0x56476e1cc734 in qio_channel_fd_source_dispatch /home/alxndr/Development/qemu/general-fuzz/io/channel-watch.c:84:12 #18 0x7fca345a2897 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897) -Alex To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1890152/+subscriptions
[Bug 1913873] Re: QEMU: net: vmxnet: integer overflow may crash guest
Yes, from the trace looks same.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1913873
Title:
QEMU: net: vmxnet: integer overflow may crash guest
Status in QEMU:
New
Bug description:
* Gaoning Pan from Zhejiang University & Ant Security Light-Year Lab reported
a malloc failure
issue locates in vmxnet3_activate_device() of qemu/hw/net/vmxnet3.c NIC
emulator
* This issue is reproducible because while activating the NIC device,
vmxnet3_activate_device
does not validate guest supplied configuration values against predefined
min/max limits.
@@ -1420,6 +1420,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
vmxnet3_setup_rx_filtering(s);
/* Cache fields from shared memory */
s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
+assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);<= Did
not check if MTU is within range
VMW_CFPRN("MTU is %u", s->mtu);
s->max_rx_frags =
@@ -1473,6 +1474,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* Read rings memory locations for TX queues */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa, conf.txRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa, conf.txRingSize);
+if (size > VMXNET3_TX_RING_MAX_SIZE) { <= Did
not check TX ring size
+size = VMXNET3_TX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].tx_ring, pa, size,
sizeof(struct Vmxnet3_TxDesc), false);
@@ -1483,6 +1487,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* TXC ring */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa,
conf.compRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa,
conf.compRingSize);
+if (size > VMXNET3_TC_RING_MAX_SIZE) { <= Did
not check TC ring size
+size = VMXNET3_TC_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].comp_ring, pa, size,
sizeof(struct Vmxnet3_TxCompDesc), true);
VMXNET3_RING_DUMP(VMW_CFPRN, "TXC", i, >txq_descr[i].comp_ring);
@@ -1524,6 +1531,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RX rings */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa,
conf.rxRingBasePA[j]);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa,
conf.rxRingSize[j]);
+if (size > VMXNET3_RX_RING_MAX_SIZE) { <= Did
not check RX ring size
+size = VMXNET3_RX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >rxq_descr[i].rx_ring[j], pa, size,
sizeof(struct Vmxnet3_RxDesc), false);
VMW_CFPRN("RX queue %d:%d: Base: %" PRIx64 ", Size: %d",
@@ -1533,6 +1543,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RXC ring */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa, conf.compRingBasePA);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa, conf.compRingSize);
+if (size > VMXNET3_RC_RING_MAX_SIZE) { <= Did
not check RC ring size
+size = VMXNET3_RC_RING_MAX_SIZE;
+}
This may lead to potential integer overflow OR OOB buffer access
issues.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1913873/+subscriptions
Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
+-- On Sat, 23 Jan 2021, P J P wrote --+
| From: Prasad J Pandit
|
| While processing ioport command in 'fdctrl_write_dor', device
| controller may select a drive which is not initialised with a
| block device. This may result in a NULL pointer dereference.
| Add checks to avoid it.
|
| Fixes: CVE-2021-20196
| Reported-by: Gaoning Pan
| Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
| Signed-off-by: Prasad J Pandit
| ---
| hw/block/fdc.c | 11 +--
| 1 file changed, 9 insertions(+), 2 deletions(-)
|
| diff --git a/hw/block/fdc.c b/hw/block/fdc.c
| index 3636874432..13a9470d19 100644
| --- a/hw/block/fdc.c
| +++ b/hw/block/fdc.c
| @@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t
value)
| }
| }
| /* Selected drive */
| -fdctrl->cur_drv = value & FD_DOR_SELMASK;
| +if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
| +fdctrl->cur_drv = value & FD_DOR_SELMASK;
| +}
|
| fdctrl->dor = value;
| }
| @@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
| uint32_t pos;
|
| cur_drv = get_cur_drv(fdctrl);
| +if (!cur_drv->blk) {
| +FLOPPY_DPRINTF("No drive connected\n");
| +return 0;
| +}
| fdctrl->dsr &= ~FD_DSR_PWRDOWN;
| if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
| FLOPPY_DPRINTF("error: controller not ready for reading\n");
| @@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
value)
| if (pos == FD_SECTOR_LEN - 1 ||
| fdctrl->data_pos == fdctrl->data_len) {
| cur_drv = get_cur_drv(fdctrl);
| -if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| +if (cur_drv->blk == NULL
| +|| blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| BDRV_SECTOR_SIZE, 0) < 0) {
| FLOPPY_DPRINTF("error writing sector %d\n",
| fd_sector(cur_drv));
|
Ping..!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1913873] Re: QEMU: net: vmxnet: integer overflow may crash guest
** Information type changed from Private Security to Public Security
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1913873
Title:
QEMU: net: vmxnet: integer overflow may crash guest
Status in QEMU:
New
Bug description:
* Gaoning Pan from Zhejiang University & Ant Security Light-Year Lab reported
a malloc failure
issue locates in vmxnet3_activate_device() of qemu/hw/net/vmxnet3.c NIC
emulator
* This issue is reproducible because while activating the NIC device,
vmxnet3_activate_device
does not validate guest supplied configuration values against predefined
min/max limits.
@@ -1420,6 +1420,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
vmxnet3_setup_rx_filtering(s);
/* Cache fields from shared memory */
s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
+assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);<= Did
not check if MTU is within range
VMW_CFPRN("MTU is %u", s->mtu);
s->max_rx_frags =
@@ -1473,6 +1474,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* Read rings memory locations for TX queues */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa, conf.txRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa, conf.txRingSize);
+if (size > VMXNET3_TX_RING_MAX_SIZE) { <= Did
not check TX ring size
+size = VMXNET3_TX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].tx_ring, pa, size,
sizeof(struct Vmxnet3_TxDesc), false);
@@ -1483,6 +1487,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* TXC ring */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa,
conf.compRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa,
conf.compRingSize);
+if (size > VMXNET3_TC_RING_MAX_SIZE) { <= Did
not check TC ring size
+size = VMXNET3_TC_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].comp_ring, pa, size,
sizeof(struct Vmxnet3_TxCompDesc), true);
VMXNET3_RING_DUMP(VMW_CFPRN, "TXC", i, >txq_descr[i].comp_ring);
@@ -1524,6 +1531,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RX rings */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa,
conf.rxRingBasePA[j]);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa,
conf.rxRingSize[j]);
+if (size > VMXNET3_RX_RING_MAX_SIZE) { <= Did
not check RX ring size
+size = VMXNET3_RX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >rxq_descr[i].rx_ring[j], pa, size,
sizeof(struct Vmxnet3_RxDesc), false);
VMW_CFPRN("RX queue %d:%d: Base: %" PRIx64 ", Size: %d",
@@ -1533,6 +1543,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RXC ring */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa, conf.compRingBasePA);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa, conf.compRingSize);
+if (size > VMXNET3_RC_RING_MAX_SIZE) { <= Did
not check RC ring size
+size = VMXNET3_RC_RING_MAX_SIZE;
+}
This may lead to potential integer overflow OR OOB buffer access
issues.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1913873/+subscriptions
[PATCH] net: vmxnet3: validate configuration values during activate (CVE-2021-20203)
From: Prasad J Pandit
While activating device in vmxnet3_acticate_device(), it does not
validate guest supplied configuration values against predefined
minimum - maximum limits. This may lead to integer overflow or
OOB access issues. Add checks to avoid it.
Fixes: CVE-2021-20203
Buglink: https://bugs.launchpad.net/qemu/+bug/1913873
Reported-by: Gaoning Pan
Signed-off-by: Prasad J Pandit
---
hw/net/vmxnet3.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index eff299f629..4a910ca971 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1420,6 +1420,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
vmxnet3_setup_rx_filtering(s);
/* Cache fields from shared memory */
s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
+assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
VMW_CFPRN("MTU is %u", s->mtu);
s->max_rx_frags =
@@ -1473,6 +1474,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* Read rings memory locations for TX queues */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa, conf.txRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa, conf.txRingSize);
+if (size > VMXNET3_TX_RING_MAX_SIZE) {
+size = VMXNET3_TX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].tx_ring, pa, size,
sizeof(struct Vmxnet3_TxDesc), false);
@@ -1483,6 +1487,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* TXC ring */
pa = VMXNET3_READ_TX_QUEUE_DESCR64(d, qdescr_pa, conf.compRingBasePA);
size = VMXNET3_READ_TX_QUEUE_DESCR32(d, qdescr_pa, conf.compRingSize);
+if (size > VMXNET3_TC_RING_MAX_SIZE) {
+size = VMXNET3_TC_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >txq_descr[i].comp_ring, pa, size,
sizeof(struct Vmxnet3_TxCompDesc), true);
VMXNET3_RING_DUMP(VMW_CFPRN, "TXC", i, >txq_descr[i].comp_ring);
@@ -1524,6 +1531,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RX rings */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa, conf.rxRingBasePA[j]);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa, conf.rxRingSize[j]);
+if (size > VMXNET3_RX_RING_MAX_SIZE) {
+size = VMXNET3_RX_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >rxq_descr[i].rx_ring[j], pa, size,
sizeof(struct Vmxnet3_RxDesc), false);
VMW_CFPRN("RX queue %d:%d: Base: %" PRIx64 ", Size: %d",
@@ -1533,6 +1543,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
/* RXC ring */
pa = VMXNET3_READ_RX_QUEUE_DESCR64(d, qd_pa, conf.compRingBasePA);
size = VMXNET3_READ_RX_QUEUE_DESCR32(d, qd_pa, conf.compRingSize);
+if (size > VMXNET3_RC_RING_MAX_SIZE) {
+size = VMXNET3_RC_RING_MAX_SIZE;
+}
vmxnet3_ring_init(d, >rxq_descr[i].comp_ring, pa, size,
sizeof(struct Vmxnet3_RxCompDesc), true);
VMW_CFPRN("RXC queue %d: Base: %" PRIx64 ", Size: %d", i, pa, size);
--
2.29.2
[Bug 1912780] Re: QEMU: Null Pointer Failure in fdctrl_read() in hw/block/fdc.c
Upstream patch: -> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg05986.html ** Information type changed from Private Security to Public Security -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1912780 Title: QEMU: Null Pointer Failure in fdctrl_read() in hw/block/fdc.c Status in QEMU: New Bug description: [via qemu-security list] This is Gaoning Pan from Zhejiang University & Ant Security Light-Year Lab. I found a Null Pointer issue locates in fdctrl_read() in hw/block/fdc.c. This flaw allows a malicious guest user or process in a denial of service condition. This issus was discovered in the latest Qemu-5.2.0. When using floppy device, there are several choices to get specific drive in get_drv(), depending on fdctrl->cur_drv. But not all drives are initialized properly, leaving fdctrl->drives[0]->blk as NULL. So when the drive was used in blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo, BDRV_SECTOR_SIZE) at line 1918, null pointer access triggers, thus denial of service.My reproduced environment is as follows: Host: ubuntu 18.04 Guest: ubuntu 18.04 My boot command is as follows: qemu-system-x86_64 -enable-kvm -boot c -m 2G -drive format=qcow2,file=./ubuntu.img \ -nic user,hostfwd=tcp:0.0.0.0:-:22 -device floppy,unit=1,drive=mydrive \ -drive id=mydrive,file=null-co://,size=2M,format=raw,if=none -display none ASAN output is as follows: = ==14688==ERROR: AddressSanitizer: SEGV on unknown address 0x034c (pc 0x5636eee9bbaf bp 0x7ff2a53fdea0 sp 0x7ff2a53fde90 T3) ==14688==The signal is caused by a WRITE memory access. ==14688==Hint: address points to the zero page. #0 0x5636eee9bbae in blk_inc_in_flight ../block/block-backend.c:1356 #1 0x5636eee9b766 in blk_prw ../block/block-backend.c:1328 #2 0x5636eee9cd76 in blk_pread ../block/block-backend.c:1491 #3 0x5636ee1adf24 in fdctrl_read_data ../hw/block/fdc.c:1918 #4 0x5636ee1a6654 in fdctrl_read ../hw/block/fdc.c:935 #5 0x5636eebb84c8 in portio_read ../softmmu/ioport.c:179 #6 0x5636ee9848c5 in memory_region_read_accessor ../softmmu/memory.c:442 #7 0x5636ee9855c2 in access_with_adjusted_size ../softmmu/memory.c:552 #8 0x5636ee98f0b7 in memory_region_dispatch_read1 ../softmmu/memory.c:1420 #9 0x5636ee98f311 in memory_region_dispatch_read ../softmmu/memory.c:1449 #10 0x5636ee8ff64a in flatview_read_continue ../softmmu/physmem.c:2822 #11 0x5636ee8ff9e5 in flatview_read ../softmmu/physmem.c:2862 #12 0x5636ee8ffb83 in address_space_read_full ../softmmu/physmem.c:2875 #13 0x5636ee8ffdeb in address_space_rw ../softmmu/physmem.c:2903 #14 0x5636eea6a924 in kvm_handle_io ../accel/kvm/kvm-all.c:2285 #15 0x5636eea6c5e3 in kvm_cpu_exec ../accel/kvm/kvm-all.c:2531 #16 0x5636eeca492b in kvm_vcpu_thread_fn ../accel/kvm/kvm-cpus.c:49 #17 0x5636ef1bc296 in qemu_thread_start ../util/qemu-thread-posix.c:521 #18 0x7ff337c736da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da) #19 0x7ff33799ca3e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x121a3e) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV ../block/block-backend.c:1356 in blk_inc_in_flight Thread T3 created by T0 here: #0 0x7ff33c580d2f in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x37d2f) #1 0x5636ef1bc673 in qemu_thread_create ../util/qemu-thread-posix.c:558 #2 0x5636eeca4ce7 in kvm_start_vcpu_thread ../accel/kvm/kvm-cpus.c:73 #3 0x5636ee9aa965 in qemu_init_vcpu ../softmmu/cpus.c:622 #4 0x5636ee82a9b4 in x86_cpu_realizefn ../target/i386/cpu.c:6731 #5 0x5636eed002f4 in device_set_realized ../hw/core/qdev.c:886 #6 0x5636eecc59bc in property_set_bool ../qom/object.c:2251 #7 0x5636eecc0c28 in object_property_set ../qom/object.c:1398 #8 0x5636eecb6fb9 in object_property_set_qobject ../qom/qom-qobject.c:28 #9 0x5636eecc1175 in object_property_set_bool ../qom/object.c:1465 #10 0x5636eecfc286 in qdev_realize ../hw/core/qdev.c:399 #11 0x5636ee739b34 in x86_cpu_new ../hw/i386/x86.c:111 #12 0x5636ee739d6d in x86_cpus_init ../hw/i386/x86.c:138 #13 0x5636ee6f843e in pc_init1 ../hw/i386/pc_piix.c:159 #14 0x5636ee6fab1e in pc_init_v5_2 ../hw/i386/pc_piix.c:438 #15 0x5636ee1cb4a7 in machine_run_board_init ../hw/core/machine.c:1134 #16 0x5636ee9c323d in qemu_init ../softmmu/vl.c:4369 #17 0x5636edd92c71 in main ../softmmu/main.c:49 #18 0x7ff33789cb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) ==14688==ABORTING Reproducer is attached. Best regards. Gaoning Pan of Zhejiang University & Ant Security Light-Year Lab To manage
[PATCH] fdc: check drive block device before usage (CVE-2021-20196)
From: Prasad J Pandit
While processing ioport command in 'fdctrl_write_dor', device
controller may select a drive which is not initialised with a
block device. This may result in a NULL pointer dereference.
Add checks to avoid it.
Fixes: CVE-2021-20196
Reported-by: Gaoning Pan
Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
Signed-off-by: Prasad J Pandit
---
hw/block/fdc.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3636874432..13a9470d19 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t
value)
}
}
/* Selected drive */
-fdctrl->cur_drv = value & FD_DOR_SELMASK;
+if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
+fdctrl->cur_drv = value & FD_DOR_SELMASK;
+}
fdctrl->dor = value;
}
@@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
uint32_t pos;
cur_drv = get_cur_drv(fdctrl);
+if (!cur_drv->blk) {
+FLOPPY_DPRINTF("No drive connected\n");
+return 0;
+}
fdctrl->dsr &= ~FD_DSR_PWRDOWN;
if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
FLOPPY_DPRINTF("error: controller not ready for reading\n");
@@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
value)
if (pos == FD_SECTOR_LEN - 1 ||
fdctrl->data_pos == fdctrl->data_len) {
cur_drv = get_cur_drv(fdctrl);
-if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+if (cur_drv->blk == NULL
+|| blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
FLOPPY_DPRINTF("error writing sector %d\n",
fd_sector(cur_drv));
--
2.29.2
Re: About 'qemu-security' list subscription process
+-- On Fri, 15 Jan 2021, Daniel P. Berrangé wrote --+ | IOW ideally there should be some web of trust whereby some existing | member(s) knows the person/entity who is requesting acces. Other cases would | have to be evaluated case-by-case basis. * True, sounds reasonable. I'll probably start a thread on the -sec list for pending requests. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [Qemu-devel] [PATCH v2 07/11] chardev: Let IOReadHandler use unsigned type
+-- On Fri, 22 Jan 2021, Richard Purdie wrote --+ | If so can anyone point me at that change? | | I ask since CVE-2018-18438 is marked as affecting all qemu versions | (https://nvd.nist.gov/vuln/detail/CVE-2018-18438). | | If it was fixed, the version mask could be updated. If the fix wasn't deemed | worthwhile for some reason that is also fine and I can mark this one as such | in our system. I'm being told we only need one of the patches in this series | which I also don't believe as I suspect we either need the set or none of | them! | | Any info would be most welcome. -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02239.html -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02231.html * Yes, the type change fix had come up during patch reviews above, and this series implemented the change. * Series is required IIUC, didn't realise it's not merged. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v2] ide: set an upper limit to nb_sectors
From: Prasad J Pandit
Set an upper limit to number of sectors on an IDE disk media.
This is to ensure that logical block addresses (LBA) and
nb_sector arguments remain within INT_MAX range.
Suggested-by: Paolo Bonzini
Signed-off-by: Prasad J Pandit
---
hw/ide/core.c | 27 ---
include/hw/ide/internal.h | 1 +
2 files changed, 17 insertions(+), 11 deletions(-)
Update v2: add explanatory comment, define macro
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04610.html
Update v1: limit s->nb_sectors count
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04270.html
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04173.html
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b49e4cfbc6..902f51423d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1161,15 +1161,25 @@ static void ide_cfata_metadata_write(IDEState *s)
s->nsector << 9), 0x200 - 2));
}
+static void ide_set_nb_sectors(IDEState *s)
+{
+uint64_t nb_sectors;
+blk_get_geometry(s->blk, _sectors);
+
+/*
+ * Restrict total number of addressable sectors to (INT_MAX << 2),
+ * so that logical addresses (LBA) stay within INT_MAX limit.
+ */
+s->nb_sectors = MIN(nb_sectors, IDE_MAX_NB_SECTORS);
+}
+
/* called when the inserted state of the media has changed */
static void ide_cd_change_cb(void *opaque, bool load, Error **errp)
{
IDEState *s = opaque;
-uint64_t nb_sectors;
s->tray_open = !load;
-blk_get_geometry(s->blk, _sectors);
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
/*
* First indicate to the guest that a CD has been removed. That's
@@ -2475,14 +2485,12 @@ static bool ide_cd_is_medium_locked(void *opaque)
static void ide_resize_cb(void *opaque)
{
IDEState *s = opaque;
-uint64_t nb_sectors;
if (!s->identify_set) {
return;
}
-blk_get_geometry(s->blk, _sectors);
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
/* Update the identify data buffer. */
if (s->drive_kind == IDE_CFATA) {
@@ -2511,17 +2519,14 @@ int ide_init_drive(IDEState *s, BlockBackend *blk,
IDEDriveKind kind,
uint32_t cylinders, uint32_t heads, uint32_t secs,
int chs_trans, Error **errp)
{
-uint64_t nb_sectors;
-
s->blk = blk;
s->drive_kind = kind;
-
-blk_get_geometry(blk, _sectors);
s->cylinders = cylinders;
s->heads = heads;
s->sectors = secs;
s->chs_trans = chs_trans;
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
+
s->wwn = wwn;
/* The SMART values should be preserved across power cycles
but they aren't. */
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 2d09162eeb..7c436def5b 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -219,6 +219,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
/* set to 1 set disable mult support */
#define MAX_MULT_SECTORS 16
+#define IDE_MAX_NB_SECTORS ((uint64_t)INT_MAX << 2)
#define IDE_DMA_BUF_SECTORS 256
/* feature values for Data Set Management */
--
2.29.2
[PATCH] ide: set an upper limit to nb_sectors
From: Prasad J Pandit
Set an upper limit to number of sectors on an IDE disk media.
This is to ensure that logical block addresses (LBA) and
nb_sector arguments remain within INT_MAX range.
Suggested-by: Paolo Bonzini
Signed-off-by: Prasad J Pandit
---
hw/ide/core.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
Update: limit s->nb_sectors count
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04270.html
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04173.html
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b49e4cfbc6..064998804a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1161,15 +1161,21 @@ static void ide_cfata_metadata_write(IDEState *s)
s->nsector << 9), 0x200 - 2));
}
+static void ide_set_nb_sectors(IDEState *s)
+{
+uint64_t nb_sectors;
+
+blk_get_geometry(s->blk, _sectors);
+s->nb_sectors = MIN(nb_sectors, (uint64_t)INT_MAX << 2);
+}
+
/* called when the inserted state of the media has changed */
static void ide_cd_change_cb(void *opaque, bool load, Error **errp)
{
IDEState *s = opaque;
-uint64_t nb_sectors;
s->tray_open = !load;
-blk_get_geometry(s->blk, _sectors);
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
/*
* First indicate to the guest that a CD has been removed. That's
@@ -2475,14 +2481,12 @@ static bool ide_cd_is_medium_locked(void *opaque)
static void ide_resize_cb(void *opaque)
{
IDEState *s = opaque;
-uint64_t nb_sectors;
if (!s->identify_set) {
return;
}
-blk_get_geometry(s->blk, _sectors);
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
/* Update the identify data buffer. */
if (s->drive_kind == IDE_CFATA) {
@@ -2511,17 +2515,14 @@ int ide_init_drive(IDEState *s, BlockBackend *blk,
IDEDriveKind kind,
uint32_t cylinders, uint32_t heads, uint32_t secs,
int chs_trans, Error **errp)
{
-uint64_t nb_sectors;
-
s->blk = blk;
s->drive_kind = kind;
-
-blk_get_geometry(blk, _sectors);
s->cylinders = cylinders;
s->heads = heads;
s->sectors = secs;
s->chs_trans = chs_trans;
-s->nb_sectors = nb_sectors;
+ide_set_nb_sectors(s);
+
s->wwn = wwn;
/* The SMART values should be preserved across power cycles
but they aren't. */
--
2.29.2
Re: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)
+-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+
| On 18/01/21 13:29, P J P wrote:
| > +s->nb_sectors = nb_sectors & (uint64_t)INT_MAX << 2;
| > if (kind == IDE_CD) {
| > +s->nb_sectors &= (uint64_t)INT_MAX << 2;
|
| Not an &, but rather a MIN.
|
| There is also ide_resize_cb, so perhaps a new function ide_set_nb_sectors in
| hw/ide/core.c would be better.
|
| ... it doesn't hurt either to have INT_MAX << 2.
Okay, will do.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)
+-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+
| s->nb_sectors is in units of 512B, so the limit would be 4TB. The purpose
| is to limit the lba and nb_sectors arguments (which are in 2048B units) of
| ide_atapi_cmd_read_{dma,pio} to INT_MAX.
* If it's for IDE_CD type, does the patch below look okay?
===
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b49e4cfbc6..034c84b350 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1169,7 +1169,7 @@ static void ide_cd_change_cb(void *opaque, bool load,
Error **errp)
s->tray_open = !load;
blk_get_geometry(s->blk, _sectors);
-s->nb_sectors = nb_sectors;
+s->nb_sectors = nb_sectors & (uint64_t)INT_MAX << 2;
/*
* First indicate to the guest that a CD has been removed. That's
@@ -2530,6 +2530,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk,
IDEDriveKind kind,
s->smart_errors = 0;
s->smart_selftest_count = 0;
if (kind == IDE_CD) {
+s->nb_sectors &= (uint64_t)INT_MAX << 2;
blk_set_dev_ops(blk, _cd_block_ops, s);
blk_set_guest_block_size(blk, 2048);
===
* Isn't 4TB limit more for IDE_CD type? Maybe UINT32_MAX?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v3] ide: atapi: check logical block address and read size (CVE-2020-29443)
From: Prasad J Pandit
While processing ATAPI cmd_read/cmd_read_cd commands,
Logical Block Address (LBA) maybe invalid OR closer to the last block,
leading to an OOB access issues. Add range check to avoid it.
Fixes: CVE-2020-29443
Reported-by: Wenxiang Qian
Suggested-by: Paolo Bonzini
Reviewed-by: Paolo Bonzini
Signed-off-by: Prasad J Pandit
---
hw/ide/atapi.c | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
Update v3: Fix space typo/error, add Reviewed-by tag
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04159.html
-> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04173.html
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index e79157863f..b626199e3d 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -322,6 +322,8 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int
max_size)
static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
int sector_size)
{
+assert(0 <= lba && lba < (s->nb_sectors >> 2));
+
s->lba = lba;
s->packet_transfer_size = nb_sectors * sector_size;
s->elementary_transfer_size = 0;
@@ -420,6 +422,8 @@ eot:
static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
int sector_size)
{
+assert(0 <= lba && lba < (s->nb_sectors >> 2));
+
s->lba = lba;
s->packet_transfer_size = nb_sectors * sector_size;
s->io_buffer_size = 0;
@@ -973,35 +977,49 @@ static void cmd_prevent_allow_medium_removal(IDEState *s,
uint8_t* buf)
static void cmd_read(IDEState *s, uint8_t* buf)
{
-int nb_sectors, lba;
+unsigned int nb_sectors, lba;
+
+/* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
+uint64_t total_sectors = s->nb_sectors >> 2;
if (buf[0] == GPCMD_READ_10) {
nb_sectors = lduw_be_p(buf + 7);
} else {
nb_sectors = ldl_be_p(buf + 6);
}
-
-lba = ldl_be_p(buf + 2);
if (nb_sectors == 0) {
ide_atapi_cmd_ok(s);
return;
}
+lba = ldl_be_p(buf + 2);
+if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
+ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
+return;
+}
+
ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
}
static void cmd_read_cd(IDEState *s, uint8_t* buf)
{
-int nb_sectors, lba, transfer_request;
+unsigned int nb_sectors, lba, transfer_request;
+
+/* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
+uint64_t total_sectors = s->nb_sectors >> 2;
nb_sectors = (buf[6] << 16) | (buf[7] << 8) | buf[8];
-lba = ldl_be_p(buf + 2);
-
if (nb_sectors == 0) {
ide_atapi_cmd_ok(s);
return;
}
+lba = ldl_be_p(buf + 2);
+if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
+ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
+return;
+}
+
transfer_request = buf[9] & 0xf8;
if (transfer_request == 0x00) {
/* nothing */
--
2.29.2
Re: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)
+-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+ | Thank you! This looks great. | With the small spacing fix suggested by checkpatch, | Reviewed-by: Paolo Bonzini Thank you. Will send patch v3 with space typo fix. | You may add a small patch on top to clamp s->nb_sectors at (uint64_t)INT_MAX | << 2, just to be super safe. To confirm: * (uint64_t)INT_MAX << 2 is => 8589934588 ~= 8.5G sectors ? Media size would be: 8.5G * 512B(sector) => ~4TB 8.5G * 4096B(sector) => ~32TB * We are limiting IDE media size to ~4TB/~32TB ? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)
From: Prasad J Pandit
While processing ATAPI cmd_read/cmd_read_cd commands,
Logical Block Address (LBA) maybe invalid OR closer to the last block,
leading to an OOB access issues. Add range check to avoid it.
Fixes: CVE-2020-29443
Reported-by: Wenxiang Qian
Fix-suggested-by: Paolo Bonzini
Signed-off-by: Prasad J Pandit
---
hw/ide/atapi.c | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
Update v2: range check lba value early in cmd_read[_cd] routines
-> https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00151.html
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index e79157863f..35b8494dc8 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -322,6 +322,8 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int
max_size)
static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
int sector_size)
{
+assert (0 <= lba && lba < (s->nb_sectors >> 2));
+
s->lba = lba;
s->packet_transfer_size = nb_sectors * sector_size;
s->elementary_transfer_size = 0;
@@ -420,6 +422,8 @@ eot:
static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
int sector_size)
{
+assert (0 <= lba && lba < (s->nb_sectors >> 2));
+
s->lba = lba;
s->packet_transfer_size = nb_sectors * sector_size;
s->io_buffer_size = 0;
@@ -973,35 +977,49 @@ static void cmd_prevent_allow_medium_removal(IDEState *s,
uint8_t* buf)
static void cmd_read(IDEState *s, uint8_t* buf)
{
-int nb_sectors, lba;
+unsigned int nb_sectors, lba;
+
+/* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
+uint64_t total_sectors = s->nb_sectors >> 2;
if (buf[0] == GPCMD_READ_10) {
nb_sectors = lduw_be_p(buf + 7);
} else {
nb_sectors = ldl_be_p(buf + 6);
}
-
-lba = ldl_be_p(buf + 2);
if (nb_sectors == 0) {
ide_atapi_cmd_ok(s);
return;
}
+lba = ldl_be_p(buf + 2);
+if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
+ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
+return;
+}
+
ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
}
static void cmd_read_cd(IDEState *s, uint8_t* buf)
{
-int nb_sectors, lba, transfer_request;
+unsigned int nb_sectors, lba, transfer_request;
+
+/* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
+uint64_t total_sectors = s->nb_sectors >> 2;
nb_sectors = (buf[6] << 16) | (buf[7] << 8) | buf[8];
-lba = ldl_be_p(buf + 2);
-
if (nb_sectors == 0) {
ide_atapi_cmd_ok(s);
return;
}
+lba = ldl_be_p(buf + 2);
+if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
+ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
+return;
+}
+
transfer_request = buf[9] & 0xf8;
if (transfer_request == 0x00) {
/* nothing */
--
2.29.2
Re: [PATCH v2 1/1] security-process: update process information
+-- On Thu, 14 Jan 2021, Philippe Mathieu-Daudé wrote --+ | What is the status of this, is something missing? -> https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg04469.html It is up and running.^^ Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1911666] Re: ZDI-CAN-10904: QEMU Plan 9 File System TOCTOU Privilege Escalation Vulnerability
'CVE-2021-20181' is assigned to this issue by Red Hat Inc. ** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2021-20181 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1911666 Title: ZDI-CAN-10904: QEMU Plan 9 File System TOCTOU Privilege Escalation Vulnerability Status in QEMU: Confirmed Bug description: -- CVSS - 7.5: AV:L/AC:H/PR:H/UI:N/S:C/C:H/I:H/A:H -- ABSTRACT - Trend Micro's Zero Day Initiative has identified a vulnerability affecting the following products: QEMU - QEMU -- VULNERABILITY DETAILS Version tested:5.0.0-rc3 Installer file:qemu-5.0.0-rc3.tar.xz Platform tested:ubuntu 18.04 x64 desktop Analysis Basically v9fs* functions called from guest kernel are executed under specific thread(I call it main thread later). But when it calls some file related system calls, qemu uses its own coroutine thread(worker thread). Then it returns(yield return) without waiting result of system call and start to execute next v9fs* function. In v9fsmarkfidsunreclaim() function, it stores fidlist member (head of singly linked list) to its stack. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L506 And if it uses coroutine, it restore fid_list from stack and restart whole loop. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L526 v9fsclunk() function calls clunkfid() which unlink fid from list, and free it. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L2060-L2091 So if v9fsclunk() is called while v9fsmarkfidsunreclaim()'s coroutine is being executed, it restores "FREED" fidp from stack and use it. it can be reproduced with the qemu binary, which is given it can also be reproduced with own ASAN build (5.0.0-rc3 and 4.2.0 are tested) ../qemu-5.0.0-rc3/x86_64-softmmu/qemu-system-x86_64 -M pc -kernel ./bzImage -initrd ./rootfs.cpio -append "root=/dev/ram console=tty1 console=ttyS0 rdinit=/bin/sh" -nographic -enable-kvm -fsdev local,id=test_dev,path=/home/xxx/sandbox,security_model=none -device virtio-9p-pci,fsdev=test_dev,mount_tag=victim_tag $ ./do.sh expected ASAN report is printed the race is in coroutine, so the threads are the same one = ==46645==ERROR: AddressSanitizer: heap-use-after-free on address 0x61047948 at pc 0x5563d8c28f0f bp0 READ of size 2 at 0x61047948 thread T0 #0 0x5563d8c28f0e in v9fs_mark_fids_unreclaim hw/9pfs/9p.c:508 #1 0x5563d8c3e9e3 in v9fs_remove hw/9pfs/9p.c:2988 #2 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #3 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) 0x61047948 is located 8 bytes inside of 192-byte region [0x61047940,0x61047a00) freed by thread T0 here: #0 0x7fadafa5f7a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8) #1 0x5563d8c27a60 in free_fid hw/9pfs/9p.c:371 #2 0x5563d8c27fcc in put_fid hw/9pfs/9p.c:396 #3 0x5563d8c37267 in v9fs_clunk hw/9pfs/9p.c:2085 #4 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #5 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) previously allocated by thread T0 here: #0 0x7fadafa5fd28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28) #1 0x7fadaf0c8b10 in g_malloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10) #2 0x5563d8c30ecc in v9fs_attach hw/9pfs/9p.c:1412 #3 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #4 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) This vulnerability was discovered by: Ryota Shiga(@Garyo) of Flatt Security working with Trend Micro Zero Day Initiative To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1911666/+subscriptions
About 'qemu-security' list subscription process
Hello, * We have received quite a few subscription requests for the 'qemu-security' list in the last few weeks. Majority of them are rejected because we could not identify the user from merely their email-id. * I have requested them to send a subscription request email with a 'Self Introduction' to the list. * However, some of the subscribers are familiar from the qemu-devel/oss-security mailing lists. And some are corporate emails like * One of the request is pending (3+) votes/acks for OR against member subscription. How do we handle these requests? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[Bug 1911666] Re: ZDI-CAN-10904: QEMU Plan 9 File System TOCTOU Privilege Escalation Vulnerability
Requesting a CVE... ** Information type changed from Private Security to Public Security -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1911666 Title: ZDI-CAN-10904: QEMU Plan 9 File System TOCTOU Privilege Escalation Vulnerability Status in QEMU: Confirmed Bug description: -- CVSS - 7.5: AV:L/AC:H/PR:H/UI:N/S:C/C:H/I:H/A:H -- ABSTRACT - Trend Micro's Zero Day Initiative has identified a vulnerability affecting the following products: QEMU - QEMU -- VULNERABILITY DETAILS Version tested:5.0.0-rc3 Installer file:qemu-5.0.0-rc3.tar.xz Platform tested:ubuntu 18.04 x64 desktop Analysis Basically v9fs* functions called from guest kernel are executed under specific thread(I call it main thread later). But when it calls some file related system calls, qemu uses its own coroutine thread(worker thread). Then it returns(yield return) without waiting result of system call and start to execute next v9fs* function. In v9fsmarkfidsunreclaim() function, it stores fidlist member (head of singly linked list) to its stack. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L506 And if it uses coroutine, it restore fid_list from stack and restart whole loop. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L526 v9fsclunk() function calls clunkfid() which unlink fid from list, and free it. -> https://github.com/qemu/qemu/blob/f3bac27cc1e303e1860cc55b9b6889ba39dee587/hw/9pfs/9p.c#L2060-L2091 So if v9fsclunk() is called while v9fsmarkfidsunreclaim()'s coroutine is being executed, it restores "FREED" fidp from stack and use it. it can be reproduced with the qemu binary, which is given it can also be reproduced with own ASAN build (5.0.0-rc3 and 4.2.0 are tested) ../qemu-5.0.0-rc3/x86_64-softmmu/qemu-system-x86_64 -M pc -kernel ./bzImage -initrd ./rootfs.cpio -append "root=/dev/ram console=tty1 console=ttyS0 rdinit=/bin/sh" -nographic -enable-kvm -fsdev local,id=test_dev,path=/home/xxx/sandbox,security_model=none -device virtio-9p-pci,fsdev=test_dev,mount_tag=victim_tag $ ./do.sh expected ASAN report is printed the race is in coroutine, so the threads are the same one = ==46645==ERROR: AddressSanitizer: heap-use-after-free on address 0x61047948 at pc 0x5563d8c28f0f bp0 READ of size 2 at 0x61047948 thread T0 #0 0x5563d8c28f0e in v9fs_mark_fids_unreclaim hw/9pfs/9p.c:508 #1 0x5563d8c3e9e3 in v9fs_remove hw/9pfs/9p.c:2988 #2 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #3 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) 0x61047948 is located 8 bytes inside of 192-byte region [0x61047940,0x61047a00) freed by thread T0 here: #0 0x7fadafa5f7a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8) #1 0x5563d8c27a60 in free_fid hw/9pfs/9p.c:371 #2 0x5563d8c27fcc in put_fid hw/9pfs/9p.c:396 #3 0x5563d8c37267 in v9fs_clunk hw/9pfs/9p.c:2085 #4 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #5 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) previously allocated by thread T0 here: #0 0x7fadafa5fd28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28) #1 0x7fadaf0c8b10 in g_malloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10) #2 0x5563d8c30ecc in v9fs_attach hw/9pfs/9p.c:1412 #3 0x5563d98d310d in coroutine_trampoline util/coroutine-ucontext.c:115 #4 0x7fadac6396af (/lib/x86_64-linux-gnu/libc.so.6+0x586af) This vulnerability was discovered by: Ryota Shiga(@Garyo) of Flatt Security working with Trend Micro Zero Day Initiative To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1911666/+subscriptions
[ANNOUNCE] qemu-security mailing list
Hello, * QEMU project has set-up a dedicated mailing list to receive and triage all its security issues. Please see: -> https://www.qemu.org/contribute/security-process/ -> https://lists.nongnu.org/mailman/listinfo/qemu-security * If you are a security researcher OR think you've found a potential security issue in QEMU, please kindly follow the new process to report your issues. * This is a moderated mailing list. It is meant for systematic handling of QEMU security issues and coordinate their public disclosure. * Membership of this list is limited to people involved in the analysis and triage of QEMU security issues. * To report QEMU security issues you need/should not subscribe to this list. * We'd like to invite representatives of security teams who are downstream consumers of QEMU to contact the list, if they wish to participate in the triage process. * All members will be required to agree and adhere to the embargo rules and restrictions. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
+-- On Fri, 11 Dec 2020, Paolo Bonzini wrote --+ | This is not the root cause. These are the last steps before bad things | happen; the root cause is what _led_ to those last steps. In this case, the | root cause is that a read request with s->lba == -1 is mistaken for a | non-read. Read requests are able to reset s->io_buffer_index and start with | the index pointing just after the end of the sector buffer; non-read | requests instead visit the buffer just once and start with | s->io_buffer_index == 0. | | In turn, the fix is to validate: | | 1) that s->lba is in range when issuing a read request | | 2) that the size of the device is sane (e.g. the number of blocks is a | positive 32-bit integer). Yes, working on a revised patch... Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE
+-- On Fri, 27 Nov 2020, Philippe Mathieu-Daudé wrote --+
| Do not allow qemu_send_packet*() and qemu_net_queue_send()
| functions to accept packets bigger then NET_BUFSIZE.
|
| We have to put a limit somewhere. NET_BUFSIZE is defined as:
| /* Maximum GSO packet size (64k) plus plenty of room for
| * the ethernet and virtio_net headers
| */
| #define NET_BUFSIZE (4096 + 65536)
|
| +if (size > NET_BUFSIZE) {
| +return -1;
| +}
| +
/usr/include/linux/if_ether.h
#define ETH_MIN_MTU 68/* Min IPv4 MTU per RFC791 */
#define ETH_MAX_MTU 0xU /* 65535, same as IP_MAX_MTU*/
if (size < ETH_MIN_MTU || size > ETH_MAX_MTU) {
return -1;
}
Should there be similar check for minimum packet size?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v2 1/1] security-process: update process information
From: Prasad J Pandit We are about to introduce a qemu-security mailing list to report and triage QEMU security issues. Update the security process web page with new mailing list address and triage details. Signed-off-by: Prasad J Pandit --- contribute/security-process.md | 154 - 1 file changed, 95 insertions(+), 59 deletions(-) Update v2: incorporate inputs from upstream reviews -> https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00568.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00584.html diff --git a/contribute/security-process.md b/contribute/security-process.md index 1239967..13b6b97 100644 --- a/contribute/security-process.md +++ b/contribute/security-process.md @@ -3,72 +3,110 @@ title: Security Process permalink: /contribute/security-process/ --- -QEMU takes security very seriously, and we aim to take immediate action to -address serious security-related problems that involve our product. - -Please report any suspected security vulnerability in QEMU to the following -addresses. You can use GPG keys for respective receipients to communicate with -us securely. If you do, please upload your GPG public key or supply it to us -in some other way, so that we can communicate to you in a secure way, too! -Please include the tag **\[QEMU-SECURITY\]** on the subject line to help us -identify your message as security-related. - -## QEMU Security Contact List - -Please copy everyone on this list: - - Contact Person(s) | Contact Address | Company | GPG Key | GPG key fingerprint -:---|:--|:--|:-:|: - Michael S. Tsirkin| m...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0xC3503912AFBE8E67) | 0270 606B 6F3C DF3D 0B17 0970 C350 3912 AFBE 8E67 - Petr Matousek | pmato...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3E786F42C44977CA) | 8107 AF16 A416 F9AF 18F3 D874 3E78 6F42 C449 77CA - Stefano Stabellini| sstabell...@kernel.org| Independent | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x894F8F4870E1AE90) | D04E 33AB A51F 67BA 07D3 0AEA 894F 8F48 70E1 AE90 - Security Response Team | secal...@redhat.com | Red Hat Inc. | [](https://access.redhat.com/site/security/team/contact/#contact) | - Michael Roth | michael.r...@amd.com | AMD | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3353C9CEF108B584) | CEAC C9E1 5534 EBAB B82D 3FA0 3353 C9CE F108 B584 - Prasad J Pandit | p...@redhat.com | Red Hat Inc. | [](http://pool.sks-keyservers.net/pks/lookup?op=vindex=0xE2858B5AF050DE8D) | 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D - -## How to Contact Us Securely - -We use GNU Privacy Guard (GnuPG or GPG) keys to secure communications. Mail -sent to members of the list can be encrypted with public keys of all members -of the list. We expect to change some of the keys we use from time to time. -Should a key change, the previous one will be revoked. - -## How we respond - -Maintainers listed on the security reporting list operate a policy of -responsible disclosure. As such they agree that any information you share with -them about security issues that are not public knowledge is kept confidential -within respective affiliated companies. It is not passed on to any third-party, -including Xen Security Project, without your permission. - -Email sent to us is read and acknowledged with a non-automated response. For -issues that are complicated and require significant attention, we will open an -investigation and keep you informed of our progress. We might take one or more -of the following steps: +Please report any suspected security issue in QEMU to the security mailing +list at: + +* [\](https://lists.nongnu.org/mailman/listinfo/qemu-security) + +To report an issue via [GPG](https://gnupg.org/) encrypted email, please send +it to the Red Hat Product Security team at: + +* [\](https://access.redhat.com/security/team/contact/#contact) + +**Note:** after the triage, encrypted issue details shall be sent to the upstream +'qemu-security' mailing list for archival purposes. + +## How to report an issue: + +* Please include as many details as possible in the issue report. + Ex: +- QEMU version, upstream commit/tag +- Host & Guest architecture x86/Arm/PPC, 32/64 bit etc. +- Affected code area/snippets +- Stack traces, crash details +- Malicious inputs/reproducer steps etc. +- Any configurations/settings required to trigger the issue. + +* Please share the QEMU command line used to invoke a guest VM. + +* Please specify whom to acknowledge for reporting this issue. + +## How we respond: + +* Process of handling security issues comprises following steps: + + 0) **Acknowledge:** +- A non-automated response email is sent to the reporter(s) to
[PATCH v2 0/1] security-process: update with mailing list details
From: Prasad J Pandit Hello, * After upstream discussions and considering various options like LaunchPad bugs, GitLab issues etc. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg04266.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg00059.html We are about to introduce a 'qemu-security' mailing list to receive and triage upstream QEMU security issues. * Intention is to allow more community participation in handling and triaging of the QEMU security issues. * This change relieves current set of individual contacts from the responsibility of handling upstream QEMU issues. Of course they are welcome to the new 'qemu-security' mailing list. * To simplify the encrypted communication process, we keep only a single contact of from our previous list of contacts. This way reporters need not look for and manage GPG keys of multiple contacts. v1: * This patch v1 updates the QEMU security-process web page with detailed process. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg06234.html v2: * Incorporate further inputs and suggestions from upstream reviews. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00568.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00584.html I'd appreciate if you have any inputs and/or suggestions for this change. Thank you. -- Prasad J Pandit (1): security-process: update process information contribute/security-process.md | 154 - 1 file changed, 95 insertions(+), 59 deletions(-) -- 2.28.0
Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
+-- On Wed, 2 Dec 2020, Philippe Mathieu-Daudé wrote --+ | a fair part is to ask the reporter to attach its reproducer to the private | BZ, Yes, reporters sharing/releasing it is best. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v1 1/1] security-process: update process information
+-- On Wed, 2 Dec 2020, Daniel P. Berrangé wrote --+ | > +- If issue is found to be less severe, an upstream public bug (or an | > + issue) will be created immediately. | | No need to repeat "or an issue". I think it would read more clearly as | |- If the severity of the issue is sufficiently low, an upstream public bug | may be created immediately. * Let's settle on public GitLab issues, shall we? * Tomorrow after an issue triage if someone asks where should they create a public tracker, it's better to have one definite answer, instead of choose either LaunchPad or GitLab issues. * OR is it better to have both? ie. file a public tracker anywhere as per ones convenience? * One GitLab is good I think. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v1 1/1] security-process: update process information
Hello Dan, Stefano, +-- On Wed, 2 Dec 2020, Stefano Stabellini wrote --+ | On Wed, 2 Dec 2020, Daniel P. Berrangé wrote: | > > + any third parties, including Xen Security Project, without your prior | > > + permission. | > | > Why this explicit note about the Xen project ? What if we decide to want | > a member of the Xen security team on the QEMU security mailing list so that | > we can collaborate on triage ? * While that's fair point, what I think it means is, even if members from other communities are present on the qemu-security list, any explicit communication and/or sharing of issue details/information/reproducers etc. across communities, with non-members will not happen without prior permission from the reporter(s). * Besides, that is not new text, it is from the current process page -> https://www.qemu.org/contribute/security-process/ | this is not an issue because the individual (probably me) of course | would not report anything to the Xen security team without prior | permission. +1000..., appreciate it.:) | > Any non-public information you share about security issues, is kept | > confidential between members of the QEMU security team, and a minimal | > number of supporting staff in their affliated companies. Information | > will not be disclosed to other third party organizations/individuals | > without prior permission from the reporter | | Sounds good to me Same here, will fix it. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v1 1/1] security-process: update process information
Hello Dan, +-- On Wed, 2 Dec 2020, Daniel P. Berrangé wrote --+ | > +- If issue is found to be less severe, an upstream public bug (or an | > + issue) will be created immediately. | | No need to repeat "or an issue". I think it would read more clearly as | |- If the severity of the issue is sufficiently low, an upstream public bug | may be created immediately. Okay. | > +- If issue is found to be severe, an embargo process below is followed, | > + and public bug (or an issue) will be opened at the end of the set | > + embargo period. | |- If the severity of the issue requires co-ordinated disclosure at a future | date, then the embargo process below is followed, and public bug will be | opened at the end of the set embargo period. Okay. | Somewhere around here is probably a good place to link to: | | https://www.qemu.org/docs/master/system/security.html | | which describes why we'll consider some things to be not security issues Towards the end, there's a section about 'How impact & severity of an issue is decided', above link will fit in there good I think. | > -If a security issue is reported that is not already publicly disclosed, an | > -embargo date may be assigned and communicated to the reporter. Embargo | > -periods will be negotiated by mutual agreement between members of the security | > -team and other relevant parties to the problem. Members of the security contact | > -list agree not to publicly disclose any details of the security issue until | > -the embargo date expires. | > +* If a security issue is reported that is not already public and is severe | > + enough, an embargo date may be assigned and communicated to the | > + reporter(s). | | | * If a security issue is reported that is not already public and its | severity requires coordinated disclosure, an embargo date may be | assigned and communicated to the reporter(s). ... | "The preferred embargo period will be upto 2 weeks, however, longer |embargoes can be negotiated if the severity of the issues requires it." Okay, will add above changes. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v1 1/1] security-process: update process information
+-- On Wed, 2 Dec 2020, Philippe Mathieu-Daudé wrote --+ | Maybe: | | 0) **Acknowledge reception** |- A non-automated response email is sent to acknowledge the | reception of the request. | This is the starting date for the maximum **60 days** required | to process the issue, including bullets 1) and 2). | | > +- Create an upstream fix patch | | with the proper Buglink/CVE/Reported-by tags. | |- Participate in the review process until the patch is merged. | Test the fix updates with the private reproducer if required. |- Close the upstream [bug] with 'Fix released', including the | commit SHA-1 of the fix. ... | > Email sent to us is read and acknowledged with a non-automated response. For | > issues that are complicated and require significant attention, we will open an | |^^^ You can remove that, as now covered by bullet 0). Okay, will do. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
Hi, [doing a combined reply] +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+ | Is it possible to release the reproducer to the community, so we can work on | a fix and test it? * No, we can not release/share reproducers on a public list. * We can request reporters to do so by their volition. | What happens to reproducers when a CVE is assigned, but the bug is marked as | "out of the QEMU security boundary"? | +-- On Tue, 1 Dec 2020, Peter Maydell wrote --+ | Also, why are we assigning CVEs for bugs we don't consider security bugs? * We need to mark these componets 'out of security scope' at the source level, rather than on each bug. * Easiest could be to just have a list of such components in the git text file. At least till the time we device --security build and run time option discussed earlier. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg04680.html +-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+ | qtests are not just helpful. Adding regression tests for bugs is a *basic* | software engineering principle. If you don't have time to write tests, you | (or your organization) should find it. * I've been doing the patch work out of my own interest. * We generally rely on upstream/engineering for fix patches, because of our narrower understanding of the code base. +-- On Wed, 2 Dec 2020, Markus Armbruster wrote --+ | Paolo Bonzini writes: | > you need at least to enclose the reproducer, otherwise you're posting a | > puzzle not a patch. :) | | Indeed. Posting puzzles is a negative-sum game.] * Agreed. I think the upcoming 'qemu-security' list may help in this regard. As issue reports+reproducer details shall go there. * Even then, we'll need to ask reporter's permission before sharing their reproducers on a public list OR with non-members. * Best is if reporters share/release reproducers themselves. Maybe we can have a public git repository and they can send a PR to include their reproducers in the repository. * That way multiple reproducers for the same issue can be held together. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v1 1/1] security-process: update process information
Hello Konrad, all +-- On Tue, 1 Dec 2020, Konrad Rzeszutek Wilk wrote --+ | On Mon, Nov 30, 2020 at 07:19:07PM +0530, P J P wrote: | > We are about to introduce a qemu-security mailing list to report | > and triage QEMU security issues. | > Update the QEMU security process web page with new mailing list | > and triage details. | | Thank you for doing it! | Reviewed-by: Konrad Rzeszutek Wilk Thank you. | with one change below. | | > +- Request a CVE and open an upstream | > + [bug](https://bugs.launchpad.net/qemu/+bug/) | > + or a [GitLab](https://gitlab.com/groups/qemu-project/-/issues) issue | | You may want to clarify that this step in the process will not disclose the | details of the issue to the public. Yes, this is covered in the following process text and under publication embargo section: === + * We aim to process ... 60 days ... After the triaging step above + +- If issue is found to be less severe, an upstream public bug (or an + issue) will be created immediately. +- If issue is found to be severe, an embargo process below is followed, + and public bug (or an issue) will be opened at the end of the set + embargo period. ... +* Embargo periods will be negotiated by mutual agreement between reporter(s), + members of the security list and other relevant parties to the problem. + Such embargo period is generally upto [2 weeks] + +* Members of the security list agree not to publicly disclose any details of + an embargoed security issue until its embargo date expires. === Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
Hello Paolo,
+-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+
| 1) Obviously this condition was not in the mind of whoever wrote the code.
| For this reason the first thing to understand if how the bug came to happen,
| which precondition was not respected. Your backtraces shows that you came
| from ide_atapi_cmd_read_pio, so:
|
| - ide_atapi_cmd_reply_end is first entered with s->io_buffer_index ==
| s->cd_sector_size
|
| - s->lba is assumed to be != -1. from there you go to cd_read_sector ->
| cd_read_sector_cb and then reenter ide_atapi_cmd_reply_end with
| s->io_buffer_index == 0. Or to cd_read_sector_sync and then continue down
| ide_atapi_cmd_reply_end, again with s->io_buffer_index == 0
|
| - if s->elementary_transfer_size > 0, the number of bytes that are read is
| bounded to s->cd_sector_size - s->io_buffer_index
|
| - if s->elementary_transfer_size == 0, the size is again bounded to
| s->cd_sector_size - s->io_buffer_index in this code:
|
| /* we cannot transmit more than one sector at a time */
| if (s->lba != -1) {
| if (size > (s->cd_sector_size - s->io_buffer_index))
| size = (s->cd_sector_size - s->io_buffer_index);
| }
|
| So my understanding is that, for the bug to happen, you need to enter
| ide_atapi_cmd_reply_end with s->lba == -1 despite being in the read CD path.
| This might be possible by passing 0x as the LBA in cmd_read.
| The correct fix would be to check lba against the media size in cmd_read.
Oh, okay.
| This is reasoning that you should understand even before starting to write a
| patch. Did you do this step?
...
| 2)... So if you did do step 1, you need to explain it to me, because at this
| point you know this part of the code better than I do. This is a step that
| you did not do, because your commit message has no such explanation.
-> https://tc.gts3.org/cs3210/2016/spring/r/hardware/ATA8-ACS.pdf
Section #7.22 Packet command
* Yes, I tried to follow the code with the above comand description as
reference.
* Because io_index was running past io_buffer, I was thikning around right
lengths and sizes. Above specification mentions about 'Byte Count Limit' and
that data transfer can not exceed that limit.
* I was thinking about checking 'elementary_transfer_size' against
'byte_count_limit', but that did not work out. The loop is confusing there,
it first sets elementary_size = size and subtracts the same
* 's->lba == -1' did not strike me as wrong, because code explicitly checks it
and gets past it. It does not flag an error when 's->lba == -1'.
| If so, no problem---I still believe the patch is incorrect, but please
| explain how my reasoning is wrong and we'll take it from there. If not, how
| do you know your patch is not papering over a bigger issue somewhere?
* I tested the patch with a reproducer and it helped to fix the crash.
* My doubt about the patch was that it prematurely ends the while loop ie.
before s->packet_transfer_size reaches zero(0), there may be possible data
loss.
| 3) We also need to ensure that the bug will not happen again. Did you get a
| reproducer from the reporter? If not, how did you trust the report to be
| correct? If so, did you try to include it in the QEMU qtest testsuite?
* I did test it against a reproducer, but did not get to the qtest part for
the time constraints.
| If my understanding of the bug is correct, the patch is not the correct fix.
| The correct fix is to add an assertion here *and* to fix the incorrect
| assumption up in the call chain. For example:
|
| - add an assertion in ide_atapi_cmd_read_{dma,pio} that s->lba <=
| s->nb_sectors && s->lba != -1
|
| - add a range check in cmd_read and cmd_read_cd similar to what is already
| done in cmd_seek (but checking the full range from lba to lba+nb_sectors-1.
Okay, will do.
| Even if the patch were correct, however, bullets (2) and (3) are two reasons
| why this patch is not acceptable for QEMU's standards---not even for a
| security fix.
|
| This is nothing new. Even though I have sometimes applied (or redone_ your
| fixes, I have told you a variation of the above every time I saw a a patch of
| yours. The output of "git log --author pjp tests" tells me you didn't heed
| the advice though; I'm calling you out this time because it's especially clear
| that you didn't do these steps and because the result is especially wrong.
* While I understand and agree that having qtests is greatly helpful, I could
not get to it due to time/cycles constraints.
* It's certainly not that I purposely did not heed the advice/suggestions.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v1 1/1] security-process: update process information
From: Prasad J Pandit We are about to introduce a qemu-security mailing list to report and triage QEMU security issues. Update the QEMU security process web page with new mailing list and triage details. Signed-off-by: Prasad J Pandit --- contribute/security-process.md | 134 - 1 file changed, 80 insertions(+), 54 deletions(-) Update v1: incorporate feedback from review to include more details -> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg06234.html diff --git a/contribute/security-process.md b/contribute/security-process.md index 1239967..fe1bc8b 100644 --- a/contribute/security-process.md +++ b/contribute/security-process.md @@ -3,43 +3,70 @@ title: Security Process permalink: /contribute/security-process/ --- -QEMU takes security very seriously, and we aim to take immediate action to -address serious security-related problems that involve our product. - -Please report any suspected security vulnerability in QEMU to the following -addresses. You can use GPG keys for respective receipients to communicate with -us securely. If you do, please upload your GPG public key or supply it to us -in some other way, so that we can communicate to you in a secure way, too! -Please include the tag **\[QEMU-SECURITY\]** on the subject line to help us -identify your message as security-related. - -## QEMU Security Contact List - -Please copy everyone on this list: - - Contact Person(s) | Contact Address | Company | GPG Key | GPG key fingerprint -:---|:--|:--|:-:|: - Michael S. Tsirkin| m...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0xC3503912AFBE8E67) | 0270 606B 6F3C DF3D 0B17 0970 C350 3912 AFBE 8E67 - Petr Matousek | pmato...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3E786F42C44977CA) | 8107 AF16 A416 F9AF 18F3 D874 3E78 6F42 C449 77CA - Stefano Stabellini| sstabell...@kernel.org| Independent | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x894F8F4870E1AE90) | D04E 33AB A51F 67BA 07D3 0AEA 894F 8F48 70E1 AE90 - Security Response Team | secal...@redhat.com | Red Hat Inc. | [](https://access.redhat.com/site/security/team/contact/#contact) | - Michael Roth | michael.r...@amd.com | AMD | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3353C9CEF108B584) | CEAC C9E1 5534 EBAB B82D 3FA0 3353 C9CE F108 B584 - Prasad J Pandit | p...@redhat.com | Red Hat Inc. | [](http://pool.sks-keyservers.net/pks/lookup?op=vindex=0xE2858B5AF050DE8D) | 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D - -## How to Contact Us Securely - -We use GNU Privacy Guard (GnuPG or GPG) keys to secure communications. Mail -sent to members of the list can be encrypted with public keys of all members -of the list. We expect to change some of the keys we use from time to time. -Should a key change, the previous one will be revoked. - -## How we respond - -Maintainers listed on the security reporting list operate a policy of -responsible disclosure. As such they agree that any information you share with -them about security issues that are not public knowledge is kept confidential -within respective affiliated companies. It is not passed on to any third-party, -including Xen Security Project, without your permission. +Please report any suspected security issue in QEMU to the security mailing +list at: + +* [\](https://lists.gnu.org/archive/html/qemu-security/) + +To report an issue via [GPG](https://gnupg.org/) encrypted email, please send +it to the Red Hat Product Security team at: + +* [\](https://access.redhat.com/security/team/contact/#contact) + +**Note:** after the triage, encrypted issue details shall be sent to the upstream +'qemu-security' mailing list for archival purposes. + +## How to report an issue: + +* Please include as many details as possible in the issue report. + Ex: +- QEMU version, upstream commit/tag +- Host & Guest architecture x86/Arm/PPC, 32/64 bit etc. +- Affected code area/snippets +- Stack traces, crash details +- Malicious inputs/reproducer steps etc. +- Any configurations/settings required to trigger the issue. + +* Please share the QEMU command line used to invoke a guest VM. + +* Please specify whom to acknowledge for reporting this issue. + +## How we respond: + +* Process of handling security issues can be divided in two halves. + + 1) **Triage:** +- Examine the issue details and confirm whether the issue is genuine +- Validate if it can be misused for malicious purposes +- Determine its worst case impact and severity + [Low/Moderate/Important/Critical] + + 2) **Response:** +- Negotiate embargo timeline (if required, depending on severity) +- Request a CVE and open an upstream +
[PATCH v1 0/1] security-process: update with mailing list details
From: Prasad J Pandit Hello, * After upstream discussions and considering various options like LaunchPad bugs, GitLab issues etc. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg04266.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg00059.html We are about to introduce a 'qemu-security' mailing list to receive and triage upstream QEMU security issues. * Intention is to allow more community participation in handling and triaging of the QEMU security issues. * This change relieves current set of individual contacts from the responsibility of handling upstream QEMU issues. Of course they are welcome to the new 'qemu-security' mailing list. * To simplify the encrypted communication process, we keep only a single contact of from our previous list of contacts. This way reporters need not look for and manage GPG keys of multiple contacts. v1 * This patch v1 updates the QEMU security-process web page with detailed process. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg06234.html I'd appreciate if you have any inputs and/or suggestions for this change. Thank you. -- Prasad J Pandit (1): security-process: update process information contribute/security-process.md | 134 - 1 file changed, 80 insertions(+), 54 deletions(-) -- 2.28.0
Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
+-- On Wed, 18 Nov 2020, P J P wrote --+
| During data transfer via packet command in 'ide_atapi_cmd_reply_end'
| 's->io_buffer_index' could exceed the 's->io_buffer' length, leading
| to OOB access issue. Add check to avoid it.
| ...
| #9 ahci_pio_transfer ../hw/ide/ahci.c:1383
| #10 ide_transfer_start_norecurse ../hw/ide/core.c:553
| #11 ide_atapi_cmd_reply_end ../hw/ide/atapi.c:284
| #12 ide_atapi_cmd_read_pio ../hw/ide/atapi.c:329
| #13 ide_atapi_cmd_read ../hw/ide/atapi.c:442
| #14 cmd_read ../hw/ide/atapi.c:988
| #15 ide_atapi_cmd ../hw/ide/atapi.c:1352
| #16 ide_transfer_start ../hw/ide/core.c:561
| #17 cmd_packet ../hw/ide/core.c:1729
| #18 ide_exec_cmd ../hw/ide/core.c:2107
| #19 handle_reg_h2d_fis ../hw/ide/ahci.c:1267
| #20 handle_cmd ../hw/ide/ahci.c:1318
| #21 check_cmd ../hw/ide/ahci.c:592
| #22 ahci_port_write ../hw/ide/ahci.c:373
| #23 ahci_mem_write ../hw/ide/ahci.c:513
|
| Reported-by: Wenxiang Qian
| Signed-off-by: Prasad J Pandit
| ---
| hw/ide/atapi.c | 3 +++
| 1 file changed, 3 insertions(+)
|
| diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
| index 14a2b0bb2f..b991947c5c 100644
| --- a/hw/ide/atapi.c
| +++ b/hw/ide/atapi.c
| @@ -276,6 +276,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
| s->packet_transfer_size -= size;
| s->elementary_transfer_size -= size;
| s->io_buffer_index += size;
| +if (s->io_buffer_index > s->io_buffer_total_len) {
| +return;
| +}
|
| /* Some adapters process PIO data right away. In that case, we need
| * to avoid mutual recursion between ide_transfer_start
|
Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v4 0/9] memory: assert and define MemoryRegionOps callbacks
+-- On Tue, 15 Sep 2020, P J P wrote --+ | +-- On Tue, 11 Aug 2020, P J P wrote --+ | | * This series asserts that MemoryRegionOps objects define read/write | | callback methods. Thus avoids potential NULL pointer dereference. | | ex. -> https://git.qemu.org/?p=qemu.git;a=commit;h=bb15013ef34617eb1344f5276292cadd326c21b2 | | | | * Also adds various undefined MemoryRegionOps read/write functions | | to avoid potential assert failure. | | | | Thank you. | | -- | | Prasad J Pandit (9): | | hw/pci-host: add pci-intack write method | | pci-host: designware: add pcie-msi read method | | vfio: add quirk device write method | | prep: add ppc-parity write method | | nvram: add nrf51_soc flash read method | | spapr_pci: add spapr msi read method | | tz-ppc: add dummy read/write methods | | imx7-ccm: add digprog mmio write method | | memory: assert MemoryRegionOps callbacks are defined | | | | hw/misc/imx7_ccm.c | 8 | | hw/misc/tz-ppc.c | 14 ++ | | hw/nvram/nrf51_nvm.c | 10 ++ | | hw/pci-host/designware.c | 19 +++ | | hw/pci-host/prep.c | 8 | | hw/ppc/prep_systemio.c | 8 | | hw/ppc/spapr_pci.c | 14 -- | | hw/vfio/pci-quirks.c | 8 | | softmmu/memory.c | 10 +- | | 9 files changed, 96 insertions(+), 3 deletions(-) | | Ping...@Peter, @David, @Paolo, @Herve, @Alex, | | * This patch series is not pulled/merged yet, could you please help with it? Ping..! -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [RFC 1/1] security-process: update process information
Hello Darren, all +-- On Tue, 24 Nov 2020, Darren Kenny wrote --+ | I always understood triage to be the initial steps in assessing a bug: | | - determining if it is a security bug, in this case | - then deciding on the severity of it | | I would not expect triage to include seeing it through to the point | where there is a fix as in the steps above and as such that definition | of triage should probably have a shorter time frame. * Yes, initial triage is to determine if a given issue is a security one and its impact if so. * After above step, an upstream bug (or GitLab issue) shall be filed if the issue can be made public readily and does not need an embargo period. * Following step about creating a patch is needed considering the influx of these issues. If such a patch is not proposed at this time, we risk having numerous CVE bugs open and unfixed without a patch. * Sometimes proposed patches take long time to get merged upstream. Hence the 60 days time frame. * It does not mean issue report will remain private for 60 days, nope. | But, if it is a security bug - then that is when the next steps would be | taken, to (not necessarily in this order): | | - negotiate an embargo (should the predefined 60 days be insufficient) | | - don't know if you need to mention that this would include downstream | in this too, since they will be the ones most likely to need the | time to distribute a fix * Embargo period is negotiated for important/critical issues. Such embargo period is generally not more than 2 weeks. * Yes, embargo process includes notifying various downstream communities about the issue, its fix(es) and co-ordinating disclosure. | - request a CVE | - create a fix for upstream | - distros can work on bringing that back into downstream as needed, | within the embargo period | | I do feel that it is worth separating the 2 phases of triage and beyond, | but of course that is only my thoughts on it, I'm sure others will have | theirs. * Yes, I appreciate it, thanks so much for sharing. * This patch is to get the qemu-security list up and running. I'll refine the process further with above/more details as we start using it. Hope that's okay. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[RFC 1/1] security-process: update process information
From: Prasad J Pandit We are about to introduce a qemu-security mailing list to report and triage QEMU security issues. Update the QEMU security process web page with new mailing list and triage details. Signed-off-by: Prasad J Pandit --- contribute/security-process.md | 105 + 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/contribute/security-process.md b/contribute/security-process.md index 1239967..a03092c 100644 --- a/contribute/security-process.md +++ b/contribute/security-process.md @@ -3,43 +3,54 @@ title: Security Process permalink: /contribute/security-process/ --- -QEMU takes security very seriously, and we aim to take immediate action to -address serious security-related problems that involve our product. - -Please report any suspected security vulnerability in QEMU to the following -addresses. You can use GPG keys for respective receipients to communicate with -us securely. If you do, please upload your GPG public key or supply it to us -in some other way, so that we can communicate to you in a secure way, too! -Please include the tag **\[QEMU-SECURITY\]** on the subject line to help us -identify your message as security-related. - -## QEMU Security Contact List - -Please copy everyone on this list: - - Contact Person(s) | Contact Address | Company | GPG Key | GPG key fingerprint -:---|:--|:--|:-:|: - Michael S. Tsirkin| m...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0xC3503912AFBE8E67) | 0270 606B 6F3C DF3D 0B17 0970 C350 3912 AFBE 8E67 - Petr Matousek | pmato...@redhat.com | Red Hat Inc. | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3E786F42C44977CA) | 8107 AF16 A416 F9AF 18F3 D874 3E78 6F42 C449 77CA - Stefano Stabellini| sstabell...@kernel.org| Independent | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x894F8F4870E1AE90) | D04E 33AB A51F 67BA 07D3 0AEA 894F 8F48 70E1 AE90 - Security Response Team | secal...@redhat.com | Red Hat Inc. | [](https://access.redhat.com/site/security/team/contact/#contact) | - Michael Roth | michael.r...@amd.com | AMD | [](https://pgp.mit.edu/pks/lookup?op=vindex=0x3353C9CEF108B584) | CEAC C9E1 5534 EBAB B82D 3FA0 3353 C9CE F108 B584 - Prasad J Pandit | p...@redhat.com | Red Hat Inc. | [](http://pool.sks-keyservers.net/pks/lookup?op=vindex=0xE2858B5AF050DE8D) | 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D - -## How to Contact Us Securely - -We use GNU Privacy Guard (GnuPG or GPG) keys to secure communications. Mail -sent to members of the list can be encrypted with public keys of all members -of the list. We expect to change some of the keys we use from time to time. -Should a key change, the previous one will be revoked. - -## How we respond - -Maintainers listed on the security reporting list operate a policy of -responsible disclosure. As such they agree that any information you share with -them about security issues that are not public knowledge is kept confidential -within respective affiliated companies. It is not passed on to any third-party, -including Xen Security Project, without your permission. +Please report any suspected security issue in QEMU to the security mailing +list at: + +* + +To report an issue securely via GPG encrypted email, please send it to the +Red Hat Product Security team at: + +* [GPG key](https://access.redhat.com/security/team/contact/#contact) + +Note: after the triage, encrypted issue details shall be sent to the upstream +'qemu-security' list for archival purposes. + +## How to report an issue: + +* Please include as many details as possible in the issue report. + Ex: +- QEMU version, upstream commit/tag +- Host & Guest architecture x86/Arm64/PPC, 32/64 bit etc. +- Affected code area/snippets +- Stack traces, crash details +- Malicious inputs/reproducer steps etc. +- Any configurations/settings required to trigger the issue. + +* Please share the QEMU command line used to invoke the guest VM. + +* Please specify whom to acknowledge for reporting this issue. + +## How we respond: + +* Steps to triage: +- Examine and validate the issue details to confirm whether the + issue is genuine and can be misused for malicious purposes. +- Determine its worst case impact and severity(Low/M/I/Critical) +- Negotiate embargo timeline (if required) +- Request a CVE and open an upstream bug +- Create an upstream fix patch + +* Above security lists are operated by select analysts, maintainers and/or + representatives from downstream communities. + +* List members follow a **responsible disclosure** policy. Any non-public + information you share about security issues, is kept confidential within the + respective affiliated companies. Such information shall not be
[RFC 0/1] security-process: update with mailing list details
From: Prasad J Pandit Hello, * After upstream discussions and considering various options like LaunchPad bugs, GitLab issues etc. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg04266.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg00059.html We are about to introduce a 'qemu-security' mailing list to receive and triage upstream QEMU security issues. * Intention is to allow more community participation in handling and triaging of the QEMU security issues. * This change relieves current set of individual contacts from the responsibility of handling upstream QEMU issues. Of course they are welcome to the new 'qemu-security' mailing list. * To simplify the encrypted communication process, we keep only a single contact of from our previous list of contacts. This way reporters need not look for and manage GPG keys of multiple contacts. * This patch updates the QEMU security-process web page with these details. I'd appreciate if you have any inputs and/or suggestions for this change. Thank you. -- Prasad J Pandit (1): security-process: update process information contribute/security-process.md | 105 + 1 file changed, 55 insertions(+), 50 deletions(-) -- 2.28.0
[PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end
From: Prasad J Pandit
During data transfer via packet command in 'ide_atapi_cmd_reply_end'
's->io_buffer_index' could exceed the 's->io_buffer' length, leading
to OOB access issue. Add check to avoid it.
...
#9 ahci_pio_transfer ../hw/ide/ahci.c:1383
#10 ide_transfer_start_norecurse ../hw/ide/core.c:553
#11 ide_atapi_cmd_reply_end ../hw/ide/atapi.c:284
#12 ide_atapi_cmd_read_pio ../hw/ide/atapi.c:329
#13 ide_atapi_cmd_read ../hw/ide/atapi.c:442
#14 cmd_read ../hw/ide/atapi.c:988
#15 ide_atapi_cmd ../hw/ide/atapi.c:1352
#16 ide_transfer_start ../hw/ide/core.c:561
#17 cmd_packet ../hw/ide/core.c:1729
#18 ide_exec_cmd ../hw/ide/core.c:2107
#19 handle_reg_h2d_fis ../hw/ide/ahci.c:1267
#20 handle_cmd ../hw/ide/ahci.c:1318
#21 check_cmd ../hw/ide/ahci.c:592
#22 ahci_port_write ../hw/ide/ahci.c:373
#23 ahci_mem_write ../hw/ide/ahci.c:513
Reported-by: Wenxiang Qian
Signed-off-by: Prasad J Pandit
---
hw/ide/atapi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 14a2b0bb2f..b991947c5c 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -276,6 +276,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
s->packet_transfer_size -= size;
s->elementary_transfer_size -= size;
s->io_buffer_index += size;
+if (s->io_buffer_index > s->io_buffer_total_len) {
+return;
+}
/* Some adapters process PIO data right away. In that case, we need
* to avoid mutual recursion between ide_transfer_start
--
2.28.0
Re: About 'qemu-security' mailing list
Hello Dan, Stefan, +-- On Tue, 17 Nov 2020, Daniel P. Berrangé wrote --+ | On Tue, Nov 17, 2020 at 04:19:42PM +, Stefan Hajnoczi wrote: | > Dan and I tried out confidential issues and unfortunately it is | > currently too limited for our workflow. | > | > It is not possible to add non-members to a confidential issue. Members | > need at least the 'Reporter' role to view confidential issues, and then | > they can view all of them (!). | > | > This means there is no way of working on a need-to-know basis. We would | > have to give anyone who ever needs to comment on an issue access to all | > other issues :(. | > | > Dan found this open feature request from 2 years ago: | > https://gitlab.com/gitlab-org/gitlab/-/issues/20252 | > | > For now I think we should stick to email. I think email is best and easiest for all. | > I'm still concerned about the prospect of writing custom mailing list | > software and hosting it somewhere. Can we run an encrypted mailing list | > without developing the software ourselves? | | We certainly should NOT get into the business of writing or hosting | custom solutions ourselves IMHO. Even if someone volunteers to do the | work upfront, that'll inevitably turn into abandonware a few years | hence when the interested party moves onto other things. * I don't know of any list provider which supports encryption. * For custom software, there is this 'schleuder' project -> https://0xacab.org/schleuder/schleuder -> https://schleuder.org/schleuder/docs/concept.html A gpg-enabled mailing list manager with resending-capabilities. * I have not used it or played with it. | I still question whether we genuinely need encrypted mailing lists in | the first place. | | Our of all the security reports QEMU has received how many reporters | actually used GPG to encrypt their reporters, and how often did the | security team actually keep using GPG when triaging and resolving it | thereafter. | | Out of countless security issues I've dealt with across many software | projects for 10 years, there have been less than 5 occassions where | encryption was used with email by a bug reporter notifying me, and out | of those only 1 of them actually justified the use of GPG. | | For projects that did use confidential issues, they still all emailed | notifications in clear text behind the scenes regardless. | | Is it not sufficient to just use a regular mailing list by default, | and continue publish security team pgp email addrs + keys for the | few cases where pgp might be desired. * True, need & usage of encryption is debatable and difficult. * Above points and possible solution of keeping the current handful PGP keys available did come up earlier -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05213.html * At this point I think, let's get started with a regular list for now. We can still continue to explore encryption support options. @Stefanha: do we need to file a request ticket to create 'qemu-security' list? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH] hw/net/e1000e: advance desc_offset in case of null descriptor
From: Prasad J Pandit
While receiving packets via e1000e_write_packet_to_guest() routine,
'desc_offset' is advanced only when RX descriptor is processed. And
RX descriptor is not processed if it has NULL buffer address.
This may lead to an infinite loop condition. Increament 'desc_offset'
to process next descriptor in the ring to avoid infinite loop.
Reported-by: Cheol-woo Myung <330cj...@gmail.com>
Signed-off-by: Prasad J Pandit
---
hw/net/e1000e_core.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcfd46696f..3b096db3a4 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1596,13 +1596,13 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct
NetRxPkt *pkt,
(const char *) _pad, e1000x_fcs_len(core->mac));
}
}
-desc_offset += desc_size;
-if (desc_offset >= total_size) {
-is_last = true;
-}
} else { /* as per intel docs; skip descriptors with null buf addr */
trace_e1000e_rx_null_descriptor();
}
+desc_offset += desc_size;
+if (desc_offset >= total_size) {
+is_last = true;
+}
e1000e_write_rx_descr(core, desc, is_last ? core->rx_pkt : NULL,
rss_info, do_ps ? ps_hdr_len : 0, );
--
2.26.2
Re: Ramping up Continuous Fuzzing of Virtual Devices in QEMU
+-- On Thu, 22 Oct 2020, Daniel P. Berrangé wrote --+ | On Thu, Oct 22, 2020 at 12:24:16PM -0400, Alexander Bulekov wrote: | > > Once [2] lands upstream, we should see a significant uptick in oss-fuzz | > > reports, and I hope that we can develop a process to ensure these bugs | > > are properly dealt with. One option we have is to make the reports | > > public immediately and send notifications to qemu-devel. This is the | > > approach taken by some other projects on oss-fuzz, such as LLVM. Though | > > its not on oss-fuzz, bugs found by syzkaller in the kernel, are also | > > automatically sent to a public list. The question is: | > > | > > What approach should we take for dealing with bugs found on oss-fuzz? | | If we assume that a non-negligible number of fuzz bugs will be exploitable | by a malicious guest OS to break out into the host, then I think it is | likely undesirable to make them public immediately without at least a basic | human triage step to catch possibly serious security issues. * Maybe the proposed 'qemu-security' list can receive such issue reports. It is more close than qemu-devel. But it also depends on the quantum of traffic oss-fuzz generates. We don't want to flood/overwhelm qemu-security list or any other list for that matter. * Human triage is required to know potential impact of an issue before it is sent to a public list. It would not be good to send guest-to-host-escape type issues directly to a public list. * Ideally preliminary human triage should be done on the fuzzers' side. After it hits an issue, someone should have a look at it before sending an email to a list OR maintainer(s). Ex. TCG issues are generally not considered for CVE. They need not go to a security list. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: About 'qemu-security' mailing list
+-- On Tue, 20 Oct 2020, P J P wrote --+ | +-- On Fri, 16 Oct 2020, P J P wrote --+ | | * So ie. we need to: | | | | 1. Create/setup a regular non-encrypted 'qemu-security' list. | | | | 2. Invite representatives from user/downstream communities to subscribe to | | it. | | | | 3. Collect & store their PGP public keys. Also create a key for the list. | | | | 4. Write 'encrypt & email' automation tool(s) to provide encryption support. | | | | 5. Document and publish above details and list workflow on a page. | | | | ...wdyt? | Ping...! -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v2] ati: check x y display parameter values
From: Prasad J Pandit
The source and destination x,y display parameters in ati_2d_blt()
may run off the vga limits if either of s->regs.[src|dst]_[xy] is
zero. Check the parameter values to avoid potential crash.
Reported-by: Gaoning Pan
Signed-off-by: Prasad J Pandit
---
hw/display/ati_2d.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
Update v2: add [dst|src]_[xy] > 0x3fff check
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05698.html
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 23a8ae0cd8..4dc10ea795 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -75,8 +75,9 @@ void ati_2d_blt(ATIVGAState *s)
dst_stride *= bpp;
}
uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
-if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) *
-dst_stride >= end) {
+if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
+|| dst_bits + dst_x
+ + (dst_y + s->regs.dst_height) * dst_stride >= end) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return;
}
@@ -107,8 +108,9 @@ void ati_2d_blt(ATIVGAState *s)
src_bits += s->regs.crtc_offset & 0x07ff;
src_stride *= bpp;
}
-if (src_bits >= end || src_bits + src_x +
-(src_y + s->regs.dst_height) * src_stride >= end) {
+if (src_x > 0x3fff || src_y > 0x3fff || src_bits >= end
+|| src_bits + src_x
+ + (src_y + s->regs.dst_height) * src_stride >= end) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return;
}
--
2.26.2
Re: [PATCH v2] net: remove an assert call in eth_get_gso_type
+-- On Wed, 21 Oct 2020, Philippe Mathieu-Daudé wrote --+ | $ git grep qemu_log\( | wc -l | 661 | | This function seems used mostly by very old code. | It is declared in "qemu/log-for-trace.h" which looks like an internal API. | | Should we add a checkpatch rule to refuse new uses of qemu_log()? * That'll help, if it's not meant to be called directly. * Better still would be to make qemu_log() static, called only via qemu_log_mask(). That way compiler will show an error if qemu_log() is called directly. * While at it, could it be made to print '__func__' string by default? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
+-- On Wed, 21 Oct 2020, Jason Wang wrote --+ | It should not be a guest error, since guest is allowed to send a packet | other than IPV4(6). * Ah...sigh! :( * I very hesitantly used guest_error mask, since it was g_assert-ing before. To me both guest_error and log_unimp seem mismatching. Because no GSO is also valid IIUC. That's why in patch v2 I used plain qemu_log(). But plain qemu_log is also not good it seems. * I'm okay either way. Please let me know which one to use. OR I'm fine if you fix it while merging upstream too. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v2] net: remove an assert call in eth_get_gso_type
+-- On Tue, 20 Oct 2020, Peter Maydell wrote --+ | If the guest must have done something wrong to get us here: | use LOG_GUEST_ERROR +-- On Tue, 20 Oct 2020, Philippe Mathieu-Daudé wrote --+ | Not sure why you choose decimal, the usual format is "0x%04"PRIx16. Sent patch v3 with above updates. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v3] net: remove an assert call in eth_get_gso_type
From: Prasad J Pandit eth_get_gso_type() routine returns segmentation offload type based on L3 protocol type. It calls g_assert_not_reached if L3 protocol is unknown, making the following return statement unreachable. Remove the g_assert call, it maybe triggered by a guest user. Reported-by: Gaoning Pan Signed-off-by: Prasad J Pandit --- net/eth.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Update v3: use LOG_GUEST_ERROR mask and %0x04PRIx16 conversion. -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05759.html -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05752.html diff --git a/net/eth.c b/net/eth.c index 0c1d413ee2..eee77071f9 100644 --- a/net/eth.c +++ b/net/eth.c @@ -16,6 +16,7 @@ */ #include "qemu/osdep.h" +#include "qemu/log.h" #include "net/eth.h" #include "net/checksum.h" #include "net/tap.h" @@ -71,9 +72,8 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto) return VIRTIO_NET_HDR_GSO_TCPV6 | ecn_state; } } - -/* Unsupported offload */ -g_assert_not_reached(); +qemu_log_mask(LOG_GUEST_ERROR, "%s: probably not GSO frame, " +"unknown L3 protocol: 0x%04"PRIx16"\n", __func__, l3_proto); return VIRTIO_NET_HDR_GSO_NONE | ecn_state; } -- 2.26.2
Re: About 'qemu-security' mailing list
+-- On Fri, 16 Oct 2020, P J P wrote --+ | * So ie. we need to: | | 1. Create/setup a regular non-encrypted 'qemu-security' list. | | 2. Invite representatives from user/downstream communities to subscribe to | it. | | 3. Collect & store their PGP public keys. Also create a key for the list. | | 4. Write 'encrypt & email' automation tool(s) to provide encryption support. | | 5. Document and publish above details and list workflow on a page. | | ...wdyt? Ping..! (just checking; probably folks are buys with KVMForum I guess) -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH v2] net: remove an assert call in eth_get_gso_type
From: Prasad J Pandit
eth_get_gso_type() routine returns segmentation offload type based on
L3 protocol type. It calls g_assert_not_reached if L3 protocol is
unknown, making the following return statement unreachable. Remove the
g_assert call, as it maybe triggered by a guest user.
Reported-by: Gaoning Pan
Signed-off-by: Prasad J Pandit
---
net/eth.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Update v2: add qemu_log()
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
diff --git a/net/eth.c b/net/eth.c
index 0c1d413ee2..fd76e349eb 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/log.h"
#include "net/eth.h"
#include "net/checksum.h"
#include "net/tap.h"
@@ -71,9 +72,7 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t
l4proto)
return VIRTIO_NET_HDR_GSO_TCPV6 | ecn_state;
}
}
-
-/* Unsupported offload */
-g_assert_not_reached();
+qemu_log("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);
return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
}
--
2.26.2
Re: [PATCH] ati: mask x y display parameter values
+-- On Tue, 20 Oct 2020, BALATON Zoltan wrote --+
| The card has 32 bit registers with values in them interpreted differently for
| different regs. For dst_x|y lower 14 bits can be set and value should be
| interpreted as -8192:8191 according to docs. I've got this wrong because all
| guests I've tried did not actually use negative values. The Solaris driver I
| was recently shown not to work may use that so I plan to look at it and fix it
| when I'll have time.
...
| Docs aren't very clear on that but I think these cannot be negative so
| 0:8191 is valid range because it mentions that also bits 0-13 (or maybe
| 0-15, the docs have a lot of copy errors) are valid but only 0-12 are
| used for rectangles, 13-15 used only for trapezoids (which we don't
| emulate). The docs are really bad so we have to guess and see what guest
| drivers do most of the time.
* I see. Are the docs available/accessible online?
| > dst_y(=4294950914(=-16382)) + s->regs.dst_height(=16383)) overflows to => 1
| > Ie. (dst_bits + dst_x(=0) + (1) * dst_stride >= end) returns false.
|
| So maybe we should cast something (like dst_stride) to uint64_t here to
| promote everything to 64 bit and prevent it overflowing which then should
| catch this as well?
...
| > +if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
| > +|| dst_bits + dst_x + (dst_y + s->regs.dst_height)
| > + * dst_stride >= end) {
| > ...
| > +if (src_x > 0x3fff || src_y > 0x3ff || src_bits >= end
| > +|| src_bits + src_x + (src_y + s->regs.dst_height)
| > + * src_stride >= end) {
| > qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
|
| I can live with that until I have a chance to rewrite it but are you sure this
| will catch all possible overflows with all vram sizes that can be set with
| vgamem_mb property?
* Considering all fields are 'uint32_t' type; And majority of the values
s->regs.[src|dst]_[xy], s->regs.dst_height are masked with '0x3fff', it
should help to avoid overflows.
* Not sure about all vram sizes. What are possible/supported size options?
* Between casting expression to 64 bits & explicit src_[xy] > 0x3fff check,
I'd go with explicit check, as it's easy to follow.
Will send a revised patch with src_[xy] > 0x3fff if it's okay with you.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] net: remove an assert call in eth_get_gso_type
+-- On Tue, 20 Oct 2020, Philippe Mathieu-Daudé wrote --+
| Maybe LOG_UNIMP with useful fields, so when user send bug report we directly
| know what has to be implemented.
qemu_log("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);
Maybe just qemu_log()? LOG_UNIMP seems mismatching.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
[PATCH] net: remove an assert call in eth_get_gso_type
From: Prasad J Pandit eth_get_gso_type() routine returns segmentation offload type to use based on L3 protocol type. It calls g_assert_not_reached if L3 protocol is unknown, making the following return statement unreachable. Remove the g_assert call, as it maybe triggered by a guest user. Reported-by: Gaoning Pan Signed-off-by: Prasad J Pandit --- net/eth.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/net/eth.c b/net/eth.c index 0c1d413ee2..f36a418077 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,9 +72,6 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto) } } -/* Unsupported offload */ -g_assert_not_reached(); - return VIRTIO_NET_HDR_GSO_NONE | ecn_state; } -- 2.26.2
