| Message ID | 1461922955-16207-1-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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;
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(+)