From e9de2e5fd602c4f5ddf212d3837b19ad4f5878ad Mon Sep 17 00:00:00 2001 From: David Vrabel Date: Tue, 2 Sep 2014 15:21:29 +0100 Subject: xen/gntalloc: fix oops after runnning out of grant refs Only set gref->gref_id if foreign access was successfully granted and the grant ref is valid. If gref->gref_id == -ENOSPC the test in __del_gref() would incorrectly attempt to end foreign access (because grant_ref_t is unsigned). Signed-off-by: David Vrabel Reported-by: Dave Scott Reviewed-by: Boris Ostrovsky --- drivers/xen/gntalloc.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index 787d1794541..8ed2bb4f6f2 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -141,13 +141,11 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op, goto undo; /* Grant foreign access to the page. */ - gref->gref_id = gnttab_grant_foreign_access(op->domid, + rc = gnttab_grant_foreign_access(op->domid, pfn_to_mfn(page_to_pfn(gref->page)), readonly); - if ((int)gref->gref_id < 0) { - rc = gref->gref_id; + if (rc < 0) goto undo; - } - gref_ids[i] = gref->gref_id; + gref_ids[i] = gref->gref_id = rc; } /* Add to gref lists. */ @@ -193,7 +191,7 @@ static void __del_gref(struct gntalloc_gref *gref) gref->notify.flags = 0; - if (gref->gref_id > 0) { + if (gref->gref_id) { if (gnttab_query_foreign_access(gref->gref_id)) return; -- cgit v1.2.3-70-g09d2 From 5903c6bd1a48d90b99e207ec2a6a7673cbbb732d Mon Sep 17 00:00:00 2001 From: David Vrabel Date: Tue, 2 Sep 2014 15:21:30 +0100 Subject: xen/gntalloc: safely delete grefs in add_grefs() undo path If a gref could not be added (perhaps because the limit has been reached or there are no more grant references available), the undo path may crash because __del_gref() frees the gref while it is being used for a list iteration. A comment suggests that using list_for_each_entry() is safe since the gref isn't removed from the list being iterated over, but it is freed and thus list_for_each_entry_safe() must be used. Also, explicitly delete the gref from the local per-file list, even though this is not strictly necessary. Signed-off-by: David Vrabel Reviewed-by: Boris Ostrovsky --- drivers/xen/gntalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index 8ed2bb4f6f2..e53fe191738 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -124,7 +124,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op, int i, rc, readonly; LIST_HEAD(queue_gref); LIST_HEAD(queue_file); - struct gntalloc_gref *gref; + struct gntalloc_gref *gref, *next; readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE); rc = -ENOMEM; @@ -160,8 +160,8 @@ undo: mutex_lock(&gref_mutex); gref_size -= (op->count - i); - list_for_each_entry(gref, &queue_file, next_file) { - /* __del_gref does not remove from queue_file */ + list_for_each_entry_safe(gref, next, &queue_file, next_file) { + list_del(&gref->next_file); __del_gref(gref); } -- cgit v1.2.3-70-g09d2