6 changed files with 246 additions and 1 deletions
@ -0,0 +1,81 @@ |
|||
From: Prasad J Pandit <address@hidden> |
|||
|
|||
Vmware Paravirtual SCSI emulation uses command descriptors to |
|||
process SCSI commands. These descriptors come with their ring |
|||
buffers. A guest could set the page count for these rings to |
|||
an arbitrary value, leading to infinite loop or OOB access. |
|||
Add check to avoid it. |
|||
|
|||
Reported-by: Tom Victor <address@hidden> |
|||
Reported-by: Li Qiang <address@hidden> |
|||
Signed-off-by: Prasad J Pandit <address@hidden> |
|||
---
|
|||
hw/scsi/vmw_pvscsi.c | 21 ++++++++++----------- |
|||
1 file changed, 10 insertions(+), 11 deletions(-) |
|||
|
|||
Update per review |
|||
-> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00019.html |
|||
|
|||
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
|
|||
index 5116f4a..4245c15 100644
|
|||
--- a/hw/scsi/vmw_pvscsi.c
|
|||
+++ b/hw/scsi/vmw_pvscsi.c
|
|||
@@ -152,7 +152,7 @@ pvscsi_log2(uint32_t input)
|
|||
return log; |
|||
} |
|||
|
|||
-static int
|
|||
+static void
|
|||
pvscsi_ring_init_data(PVSCSIRingInfo *m, PVSCSICmdDescSetupRings *ri) |
|||
{ |
|||
int i; |
|||
@@ -160,10 +160,6 @@ pvscsi_ring_init_data(PVSCSIRingInfo *m, PVSCSICmdDescSetupRings *ri)
|
|||
uint32_t req_ring_size, cmp_ring_size; |
|||
m->rs_pa = ri->ringsStatePPN << VMW_PAGE_SHIFT; |
|||
|
|||
- if ((ri->reqRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES)
|
|||
- || (ri->cmpRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES)) {
|
|||
- return -1;
|
|||
- }
|
|||
req_ring_size = ri->reqRingNumPages * PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE; |
|||
cmp_ring_size = ri->cmpRingNumPages * PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE; |
|||
txr_len_log2 = pvscsi_log2(req_ring_size - 1); |
|||
@@ -195,8 +191,6 @@ pvscsi_ring_init_data(PVSCSIRingInfo *m, PVSCSICmdDescSetupRings *ri)
|
|||
|
|||
/* Flush ring state page changes */ |
|||
smp_wmb(); |
|||
-
|
|||
- return 0;
|
|||
} |
|||
|
|||
static int |
|||
@@ -746,7 +740,7 @@ pvscsi_dbg_dump_tx_rings_config(PVSCSICmdDescSetupRings *rc)
|
|||
|
|||
trace_pvscsi_tx_rings_num_pages("Confirm Ring", rc->cmpRingNumPages); |
|||
for (i = 0; i < rc->cmpRingNumPages; i++) { |
|||
- trace_pvscsi_tx_rings_ppn("Confirm Ring", rc->reqRingPPNs[i]);
|
|||
+ trace_pvscsi_tx_rings_ppn("Confirm Ring", rc->cmpRingPPNs[i]);
|
|||
} |
|||
} |
|||
|
|||
@@ -779,10 +773,15 @@ pvscsi_on_cmd_setup_rings(PVSCSIState *s)
|
|||
|
|||
trace_pvscsi_on_cmd_arrived("PVSCSI_CMD_SETUP_RINGS"); |
|||
|
|||
+ if (!rc->reqRingNumPages
|
|||
+ || rc->reqRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES
|
|||
+ || !rc->cmpRingNumPages
|
|||
+ || rc->cmpRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
|
|||
+ return PVSCSI_COMMAND_PROCESSING_FAILED;
|
|||
+ }
|
|||
+
|
|||
pvscsi_dbg_dump_tx_rings_config(rc); |
|||
- if (pvscsi_ring_init_data(&s->rings, rc) < 0) {
|
|||
- return PVSCSI_COMMAND_PROCESSING_FAILED;
|
|||
- }
|
|||
+ pvscsi_ring_init_data(&s->rings, rc);
|
|||
|
|||
s->rings_info_valid = TRUE; |
|||
return PVSCSI_COMMAND_PROCESSING_SUCCEEDED; |
|||
--
|
|||
2.5.5 |
|||
@ -0,0 +1,62 @@ |
|||
From: Prasad J Pandit <address@hidden> |
|||
|
|||
In PVSCSI paravirtual SCSI bus, pvscsi_convert_sglist can take a very |
|||
long time or go into an infinite loop due to two different bugs: |
|||
|
|||
1) the request descriptor data length is defined to be 64 bit. While |
|||
building SG list from a request descriptor, it gets truncated to 32bit |
|||
in routine 'pvscsi_convert_sglist'. This could lead to an infinite loop |
|||
situation for large 'dataLen' values, when data_length is cast to uint32_t |
|||
and chunk_size becomes always zero. Fix this by removing the incorrect |
|||
cast. |
|||
|
|||
2) pvscsi_get_next_sg_elem can be called arbitrarily many times if the |
|||
element has a zero length. Get out of the loop early when this happens, |
|||
by introducing an upper limit on the number of SG list elements. |
|||
|
|||
Reported-by: Li Qiang <address@hidden> |
|||
Signed-off-by: Prasad J Pandit <address@hidden> |
|||
---
|
|||
hw/scsi/vmw_pvscsi.c | 11 ++++++----- |
|||
1 file changed, 6 insertions(+), 5 deletions(-) |
|||
|
|||
Update as per: |
|||
-> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01172.html |
|||
|
|||
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
|
|||
index 4245c15..babac5a 100644
|
|||
--- a/hw/scsi/vmw_pvscsi.c
|
|||
+++ b/hw/scsi/vmw_pvscsi.c
|
|||
@@ -40,6 +40,8 @@
|
|||
#define PVSCSI_MAX_DEVS (64) |
|||
#define PVSCSI_MSIX_NUM_VECTORS (1) |
|||
|
|||
+#define PVSCSI_MAX_SG_ELEM 2048
|
|||
+
|
|||
#define PVSCSI_MAX_CMD_DATA_WORDS \ |
|||
(sizeof(PVSCSICmdDescSetupRings)/sizeof(uint32_t)) |
|||
|
|||
@@ -628,17 +630,16 @@ pvscsi_queue_pending_descriptor(PVSCSIState *s, SCSIDevice **d,
|
|||
static void |
|||
pvscsi_convert_sglist(PVSCSIRequest *r) |
|||
{ |
|||
- int chunk_size;
|
|||
+ uint32_t chunk_size, elmcnt = 0;
|
|||
uint64_t data_length = r->req.dataLen; |
|||
PVSCSISGState sg = r->sg; |
|||
- while (data_length) {
|
|||
- while (!sg.resid) {
|
|||
+ while (data_length && elmcnt < PVSCSI_MAX_SG_ELEM) {
|
|||
+ while (!sg.resid && elmcnt++ < PVSCSI_MAX_SG_ELEM) {
|
|||
pvscsi_get_next_sg_elem(&sg); |
|||
trace_pvscsi_convert_sglist(r->req.context, r->sg.dataAddr, |
|||
r->sg.resid); |
|||
} |
|||
- assert(data_length > 0);
|
|||
- chunk_size = MIN((unsigned) data_length, sg.resid);
|
|||
+ chunk_size = MIN(data_length, sg.resid);
|
|||
if (chunk_size) { |
|||
qemu_sglist_add(&r->sgl, sg.dataAddr, chunk_size); |
|||
} |
|||
--
|
|||
2.5.5 |
|||
@ -0,0 +1,28 @@ |
|||
From: Prasad J Pandit <address@hidden> |
|||
|
|||
When LSI SAS1068 Host Bus emulator builds configuration page |
|||
headers, the format string used in 'mptsas_config_manufacturing_1' |
|||
was wrong. It could lead to an invalid memory access. |
|||
|
|||
Reported-by: Tom Victor <address@hidden> |
|||
Fix-suggested-by: Paolo Bonzini <address@hidden> |
|||
Signed-off-by: Prasad J Pandit <address@hidden> |
|||
---
|
|||
hw/scsi/mptconfig.c | 2 +- |
|||
1 file changed, 1 insertion(+), 1 deletion(-) |
|||
|
|||
diff --git a/hw/scsi/mptconfig.c b/hw/scsi/mptconfig.c
|
|||
index 7071854..1ec895b 100644
|
|||
--- a/hw/scsi/mptconfig.c
|
|||
+++ b/hw/scsi/mptconfig.c
|
|||
@@ -203,7 +203,7 @@ size_t mptsas_config_manufacturing_1(MPTSASState *s, uint8_t **data, int address
|
|||
{ |
|||
/* VPD - all zeros */ |
|||
return MPTSAS_CONFIG_PACK(1, MPI_CONFIG_PAGETYPE_MANUFACTURING, 0x00, |
|||
- "s256");
|
|||
+ "*s256");
|
|||
} |
|||
|
|||
static |
|||
--
|
|||
2.5.5 |
|||
@ -0,0 +1,27 @@ |
|||
From: Prasad J Pandit <address@hidden> |
|||
|
|||
When LSI SAS1068 Host Bus emulator builds configuration page |
|||
headers, mptsas_config_pack() asserts to check returned size |
|||
value is within limit of 256 bytes. Fix that assert expression. |
|||
|
|||
Suggested-by: Paolo Bonzini <address@hidden> |
|||
Signed-off-by: Prasad J Pandit <address@hidden> |
|||
---
|
|||
hw/scsi/mptconfig.c | 2 +- |
|||
1 file changed, 1 insertion(+), 1 deletion(-) |
|||
|
|||
diff --git a/hw/scsi/mptconfig.c b/hw/scsi/mptconfig.c
|
|||
index 1ec895b..531947f 100644
|
|||
--- a/hw/scsi/mptconfig.c
|
|||
+++ b/hw/scsi/mptconfig.c
|
|||
@@ -158,7 +158,7 @@ static size_t mptsas_config_pack(uint8_t **data, const char *fmt, ...)
|
|||
va_end(ap); |
|||
|
|||
if (data) { |
|||
- assert(ret < 256 && (ret % 4) == 0);
|
|||
+ assert(ret / 4 < 256);
|
|||
stb_p(*data + 1, ret / 4); |
|||
} |
|||
return ret; |
|||
--
|
|||
2.5.5 |
|||
@ -0,0 +1,40 @@ |
|||
From: Prasad J Pandit <address@hidden> |
|||
|
|||
When processing svga command DEFINE_CURSOR in vmsvga_fifo_run, |
|||
the computed BITMAP and PIXMAP size are checked against the |
|||
'cursor.mask[]' and 'cursor.image[]' array sizes in bytes. |
|||
Correct these checks to avoid OOB memory access. |
|||
|
|||
Reported-by: Qinghao Tang <address@hidden> |
|||
Reported-by: Li Qiang <address@hidden> |
|||
Signed-off-by: Prasad J Pandit <address@hidden> |
|||
---
|
|||
hw/display/vmware_vga.c | 12 +++++++----- |
|||
1 file changed, 7 insertions(+), 5 deletions(-) |
|||
|
|||
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
|
|||
index e51a05e..6599cf0 100644
|
|||
--- a/hw/display/vmware_vga.c
|
|||
+++ b/hw/display/vmware_vga.c
|
|||
@@ -676,11 +676,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
|
|||
cursor.bpp = vmsvga_fifo_read(s); |
|||
|
|||
args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp); |
|||
- if (cursor.width > 256 ||
|
|||
- cursor.height > 256 ||
|
|||
- cursor.bpp > 32 ||
|
|||
- SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
|
|||
- SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
|
|||
+ if (cursor.width > 256
|
|||
+ || cursor.height > 256
|
|||
+ || cursor.bpp > 32
|
|||
+ || SVGA_BITMAP_SIZE(x, y)
|
|||
+ > sizeof(cursor.mask) / sizeof(cursor.mask[0])
|
|||
+ || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
|
|||
+ > sizeof(cursor.image) / sizeof(cursor.image[0])) {
|
|||
goto badcmd; |
|||
} |
|||
|
|||
--
|
|||
2.5.5 |
|||
|
|||
Loading…
Reference in new issue