scsi: pvscsi: limit process IO loop to maximum page count

Commit Message

Prasad Pandit Sept. 7, 2016, 4:43 a.m. UTC

From: Prasad J Pandit <[e-mail]>

Vmware Paravirtual SCSI emulator while processing IO requests
could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
always returned positive value. Limit IO loop to the maximum
page count.

Reported-by: Li Qiang <[e-mail]>
Signed-off-by: Prasad J Pandit <[e-mail]>
---
 hw/scsi/vmw_pvscsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Prasad Pandit Sept. 13, 2016, 7 a.m. UTC | #1

+-- On Wed, 7 Sep 2016, P J P wrote --+
| From: Prasad J Pandit <[e-mail]>
| 
| Vmware Paravirtual SCSI emulator while processing IO requests
| could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
| always returned positive value. Limit IO loop to the maximum
| page count.
| 
| Reported-by: Li Qiang <[e-mail]>
| Signed-off-by: Prasad J Pandit <[e-mail]>
| ---
|  hw/scsi/vmw_pvscsi.c | 4 +++-
|  1 file changed, 3 insertions(+), 1 deletion(-)
| 
| diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
| index babac5a..3e77a08 100644
| --- a/hw/scsi/vmw_pvscsi.c
| +++ b/hw/scsi/vmw_pvscsi.c
| @@ -711,11 +711,13 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
|  static void
|  pvscsi_process_io(PVSCSIState *s)
|  {
| +    int descr_pa_cnt = 0;
|      PVSCSIRingReqDesc descr;
|      hwaddr next_descr_pa;
|  
|      assert(s->rings_info_valid);
| -    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0) {
| +    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0)
| +            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
|  
|          /* Only read after production index verification */
|          smp_rmb();

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Dmitry Fleytman Sept. 13, 2016, 8:31 a.m. UTC | #2

Hello Prasad,

Please see my questions inline.

> On 13 Sep 2016, at 10:00 AM, P J P <[e-mail]> wrote:
> 
> +-- On Wed, 7 Sep 2016, P J P wrote --+
> | From: Prasad J Pandit <[e-mail]>
> | 
> | Vmware Paravirtual SCSI emulator while processing IO requests
> | could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
> | always returned positive value. Limit IO loop to the maximum

Do you see any specific scenario why this might happen?

> | page count.
> | 
> | Reported-by: Li Qiang <[e-mail]>
> | Signed-off-by: Prasad J Pandit <[e-mail]>
> | ---
> |  hw/scsi/vmw_pvscsi.c | 4 +++-
> |  1 file changed, 3 insertions(+), 1 deletion(-)
> | 
> | diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> | index babac5a..3e77a08 100644
> | --- a/hw/scsi/vmw_pvscsi.c
> | +++ b/hw/scsi/vmw_pvscsi.c
> | @@ -711,11 +711,13 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
> |  static void
> |  pvscsi_process_io(PVSCSIState *s)
> |  {
> | +    int descr_pa_cnt = 0;
> |      PVSCSIRingReqDesc descr;
> |      hwaddr next_descr_pa;
> |  
> |      assert(s->rings_info_valid);
> | -    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0) {
> | +    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0)
> | +            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {

Why do you limit number of processed descriptors by maximal number of pages in data exchange ring?
What will happen to requests still waiting in the ring after this function exits?

Thanks,
Dmitry

> |  
> |          /* Only read after production index verification */
> |          smp_rmb();
> 
> Ping...!
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Prasad Pandit Sept. 13, 2016, 10:48 a.m. UTC | #3

Hello Dmitry,

+-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
| > On 13 Sep 2016, at 10:00 AM, P J P <[e-mail]> wrote:
| > 
| > +-- On Wed, 7 Sep 2016, P J P wrote --+
| > | From: Prasad J Pandit <[e-mail]>
| > | 
| > | Vmware Paravirtual SCSI emulator while processing IO requests
| > | could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
| > | always returned positive value. Limit IO loop to the maximum
| 
| Do you see any specific scenario why this might happen?

  A guest user could set the 'ready_ptr' and 'PVSCSIRingInfo *mgr' parameter 
in 'pvscsi_ring_pop_req_descr', such that it always returns true.

| > | Reported-by: Li Qiang <[e-mail]>
| > |  pvscsi_process_io(PVSCSIState *s)
| > |  {
| > | +    int descr_pa_cnt = 0;
| > |      PVSCSIRingReqDesc descr;
| > |      hwaddr next_descr_pa;
| > |  
| > |      assert(s->rings_info_valid);
| > | -    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0) {
| > | +    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0)
| > | +            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
| 
| Why do you limit number of processed descriptors by maximal number of pages 
| in data exchange ring? What will happen to requests still waiting in the 
| ring after this function exits?

  I limit it to maximum page count thinking that the descriptor value returned 
by pvscsi_ring_pop_req_descr() is derived from the mgr->req_ring_pages_pa[] 
array, which is of size 'PVSCSI_SETUP_RINGS_MAX_NUM_PAGES'. If 
pvscsi_process_io() was to go into an infinite loop, it'd continue processing 
the same set of req_ring_pages?

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Dmitry Fleytman Sept. 13, 2016, 12:10 p.m. UTC | #4

> On 13 Sep 2016, at 13:48 PM, P J P <[e-mail]> wrote:
> 
>  Hello Dmitry,
> 
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | > On 13 Sep 2016, at 10:00 AM, P J P <[e-mail]> wrote:
> | > 
> | > +-- On Wed, 7 Sep 2016, P J P wrote --+
> | > | From: Prasad J Pandit <[e-mail]>
> | > | 
> | > | Vmware Paravirtual SCSI emulator while processing IO requests
> | > | could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
> | > | always returned positive value. Limit IO loop to the maximum
> | 
> | Do you see any specific scenario why this might happen?
> 
>  A guest user could set the 'ready_ptr' and 'PVSCSIRingInfo *mgr' parameter 
> in 'pvscsi_ring_pop_req_descr', such that it always returns true.

I see. The problematic code is if (ready_ptr != mgr->consumed_ptr) {…}

mgr->consumed_ptr is managed by device and not visible to the driver,
but ready_ptr is managed by driver and may be set to some “big” number.

In this case it may take a lot of iterations for consumed_ptr
to become equal to ready_ptr and additionally some requests will be send multiple times.

The most straightforward way to fix this issue will be to
check that ready_ptr - consumed_ptr is less than ring size.

> 
> | > | Reported-by: Li Qiang <[e-mail]>
> | > |  pvscsi_process_io(PVSCSIState *s)
> | > |  {
> | > | +    int descr_pa_cnt = 0;
> | > |      PVSCSIRingReqDesc descr;
> | > |      hwaddr next_descr_pa;
> | > |  
> | > |      assert(s->rings_info_valid);
> | > | -    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0) {
> | > | +    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0)
> | > | +            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
> | 
> | Why do you limit number of processed descriptors by maximal number of pages 
> | in data exchange ring? What will happen to requests still waiting in the 
> | ring after this function exits?
> 
>  I limit it to maximum page count thinking that the descriptor value returned 
> by pvscsi_ring_pop_req_descr() is derived from the mgr->req_ring_pages_pa[] 
> array, which is of size 'PVSCSI_SETUP_RINGS_MAX_NUM_PAGES'. If 
> pvscsi_process_io() was to go into an infinite loop, it'd continue processing 
> the same set of req_ring_pages?

I think you’re mixing concepts of number of
pages in the ring and number of requests in the ring.

Each page contains (much) more than one request.

> 
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Prasad Pandit Sept. 13, 2016, 1:09 p.m. UTC | #5

+-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
| >  A guest user could set the 'ready_ptr' and 'PVSCSIRingInfo *mgr' parameter 
| > in 'pvscsi_ring_pop_req_descr', such that it always returns true.
| 
| I see. The problematic code is if (ready_ptr != mgr->consumed_ptr) {…}
| 
| mgr->consumed_ptr is managed by device and not visible to the driver,
| but ready_ptr is managed by driver and may be set to some “big” number.
| 
| In this case it may take a lot of iterations for consumed_ptr
| to become equal to ready_ptr and additionally some requests will be send multiple times.
| 
| The most straightforward way to fix this issue will be to
| check that ready_ptr - consumed_ptr is less than ring size.

  I see.

| I think you’re mixing concepts of number of
| pages in the ring and number of requests in the ring.
| 
| Each page contains (much) more than one request.

  I see, okay.

Thank you so much for the details. I'll send a revised patch.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Patch

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index babac5a..3e77a08 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -711,11 +711,13 @@  pvscsi_process_request_descriptor(PVSCSIState *s,
 static void
 pvscsi_process_io(PVSCSIState *s)
 {
+    int descr_pa_cnt = 0;
     PVSCSIRingReqDesc descr;
     hwaddr next_descr_pa;
 
     assert(s->rings_info_valid);
-    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0) {
+    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 0)
+            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
 
         /* Only read after production index verification */
         smp_rmb();