* [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
@ 2016-09-07 4:43 P J P
2016-09-13 7:00 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-09-07 4:43 UTC (permalink / raw)
To: Qemu Developers; +Cc: Paolo Bonzini, Dmitry Fleytman, Li Qiang, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
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 <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
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();
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
2016-09-07 4:43 [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count P J P
@ 2016-09-13 7:00 ` P J P
2016-09-13 8:31 ` Dmitry Fleytman
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-09-13 7:00 UTC (permalink / raw)
To: Qemu Developers; +Cc: Dmitry Fleytman, Paolo Bonzini, Li Qiang
+-- On Wed, 7 Sep 2016, P J P wrote --+
| From: Prasad J Pandit <pjp@fedoraproject.org>
|
| 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 <liqiang6-s@360.cn>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
| 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
2016-09-13 7:00 ` P J P
@ 2016-09-13 8:31 ` Dmitry Fleytman
2016-09-13 10:48 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Fleytman @ 2016-09-13 8:31 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Paolo Bonzini, Li Qiang
Hello Prasad,
Please see my questions inline.
> On 13 Sep 2016, at 10:00 AM, P J P <ppandit@redhat.com> wrote:
>
> +-- On Wed, 7 Sep 2016, P J P wrote --+
> | From: Prasad J Pandit <pjp@fedoraproject.org>
> |
> | 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 <liqiang6-s@360.cn>
> | Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | ---
> | 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
2016-09-13 8:31 ` Dmitry Fleytman
@ 2016-09-13 10:48 ` P J P
2016-09-13 12:10 ` Dmitry Fleytman
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-09-13 10:48 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Qemu Developers, Paolo Bonzini, Li Qiang
Hello Dmitry,
+-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
| > On 13 Sep 2016, at 10:00 AM, P J P <ppandit@redhat.com> wrote:
| >
| > +-- On Wed, 7 Sep 2016, P J P wrote --+
| > | From: Prasad J Pandit <pjp@fedoraproject.org>
| > |
| > | 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 <liqiang6-s@360.cn>
| > | 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
2016-09-13 10:48 ` P J P
@ 2016-09-13 12:10 ` Dmitry Fleytman
2016-09-13 13:09 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Fleytman @ 2016-09-13 12:10 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Paolo Bonzini, Li Qiang
> On 13 Sep 2016, at 13:48 PM, P J P <ppandit@redhat.com> wrote:
>
> Hello Dmitry,
>
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | > On 13 Sep 2016, at 10:00 AM, P J P <ppandit@redhat.com> wrote:
> | >
> | > +-- On Wed, 7 Sep 2016, P J P wrote --+
> | > | From: Prasad J Pandit <pjp@fedoraproject.org>
> | > |
> | > | 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 <liqiang6-s@360.cn>
> | > | 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count
2016-09-13 12:10 ` Dmitry Fleytman
@ 2016-09-13 13:09 ` P J P
0 siblings, 0 replies; 6+ messages in thread
From: P J P @ 2016-09-13 13:09 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Qemu Developers, Paolo Bonzini, Li Qiang
+-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-13 13:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 4:43 [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count P J P
2016-09-13 7:00 ` P J P
2016-09-13 8:31 ` Dmitry Fleytman
2016-09-13 10:48 ` P J P
2016-09-13 12:10 ` Dmitry Fleytman
2016-09-13 13:09 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.