[PATCH v2] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[PATCH v2] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Konrad Rzeszutek Wilk]

Currently it is possible to:

1)  xc_xsplice_apply()
     \-> xsplice_action
	spin_lock(payload_lock)
             \- schedule_work()
        spin_unlock(payload_lock);

2)  xc_xsplice_unload()
     \-> xsplice_action
	spin_lock(payload_lock)
             free_payload(data);
        spin_unlock(payload_lock);

.. all CPUs are quiesced.

3) check_for_xsplice_work()
     \-> apply_payload
        \-> arch_xsplice_apply_jmp
		BOOM

The reason is that state is in 'CHECKED' which changes to 'APPLIED'
once check_for_xsplice_work finishes. So we have a race between 1) -> 3)
where one can manipulate the payload.

To guard against this we add a check in xsplice_action to not allow
any actions if schedule_work has been called for this specific payload.

The function 'is_work_scheduled' checks xsplice_work which is safe as:
 - The ->do_work changes to 1 under the payload_lock (which we also hold).
 - The ->do_work changes to 0 when all CPUs are quisced and IRQs have
   been disabled.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reported-and-Tested-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
---
---
 xen/common/xsplice.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 1b67d39..777faa7 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
     data->rc = rc;
 }
 
+static bool_t is_work_scheduled(const struct payload *data)
+{
+    ASSERT(spin_is_locked(&payload_lock));
+
+    return xsplice_work.do_work && xsplice_work.data == data;
+}
+
 static int schedule_work(struct payload *data, uint32_t cmd, uint32_t timeout)
 {
     ASSERT(spin_is_locked(&payload_lock));
@@ -1363,6 +1370,12 @@ static int xsplice_action(xen_sysctl_xsplice_action_t *action)
         return PTR_ERR(data);
     }
 
+    if ( is_work_scheduled(data) )
+    {
+        rc = -EBUSY;
+        goto out;
+    }
+
     switch ( action->cmd )
     {
     case XSPLICE_ACTION_UNLOAD:
@@ -1423,6 +1436,7 @@ static int xsplice_action(xen_sysctl_xsplice_action_t *action)
         break;
     }
 
+ out:
     spin_unlock(&payload_lock);
 
     return rc;
-- 
2.5.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Wei Liu]

On Fri, Apr 29, 2016 at 05:42:35AM -0400, Konrad Rzeszutek Wilk wrote:
> Currently it is possible to:
> 
> 1)  xc_xsplice_apply()
>      \-> xsplice_action
> 	spin_lock(payload_lock)
>              \- schedule_work()
>         spin_unlock(payload_lock);
> 
> 2)  xc_xsplice_unload()
>      \-> xsplice_action
> 	spin_lock(payload_lock)
>              free_payload(data);
>         spin_unlock(payload_lock);
> 
> .. all CPUs are quiesced.
> 
> 3) check_for_xsplice_work()
>      \-> apply_payload
>         \-> arch_xsplice_apply_jmp
> 		BOOM
> 
> The reason is that state is in 'CHECKED' which changes to 'APPLIED'
> once check_for_xsplice_work finishes. So we have a race between 1) -> 3)
> where one can manipulate the payload.
> 
> To guard against this we add a check in xsplice_action to not allow
> any actions if schedule_work has been called for this specific payload.
> 
> The function 'is_work_scheduled' checks xsplice_work which is safe as:
>  - The ->do_work changes to 1 under the payload_lock (which we also hold).
>  - The ->do_work changes to 0 when all CPUs are quisced and IRQs have
>    been disabled.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reported-and-Tested-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel