Andrew Geissler | c926e17 | 2021-05-07 16:11:35 -0500 | [diff] [blame^] | 1 | From c2d2d14e8deece958bbc4fc649d22c3564bc4e7e Mon Sep 17 00:00:00 2001 |
| 2 | From: Greg Kurz <groug@kaod.org> |
| 3 | Date: Thu, 14 Jan 2021 17:04:12 +0100 |
| 4 | Subject: [PATCH] 9pfs: Fully restart unreclaim loop (CVE-2021-20181) |
| 5 | |
| 6 | Depending on the client activity, the server can be asked to open a huge |
| 7 | number of file descriptors and eventually hit RLIMIT_NOFILE. This is |
| 8 | currently mitigated using a reclaim logic : the server closes the file |
| 9 | descriptors of idle fids, based on the assumption that it will be able |
| 10 | to re-open them later. This assumption doesn't hold of course if the |
| 11 | client requests the file to be unlinked. In this case, we loop on the |
| 12 | entire fid list and mark all related fids as unreclaimable (the reclaim |
| 13 | logic will just ignore them) and, of course, we open or re-open their |
| 14 | file descriptors if needed since we're about to unlink the file. |
| 15 | |
| 16 | This is the purpose of v9fs_mark_fids_unreclaim(). Since the actual |
| 17 | opening of a file can cause the coroutine to yield, another client |
| 18 | request could possibly add a new fid that we may want to mark as |
| 19 | non-reclaimable as well. The loop is thus restarted if the re-open |
| 20 | request was actually transmitted to the backend. This is achieved |
| 21 | by keeping a reference on the first fid (head) before traversing |
| 22 | the list. |
| 23 | |
| 24 | This is wrong in several ways: |
| 25 | - a potential clunk request from the client could tear the first |
| 26 | fid down and cause the reference to be stale. This leads to a |
| 27 | use-after-free error that can be detected with ASAN, using a |
| 28 | custom 9p client |
| 29 | - fids are added at the head of the list : restarting from the |
| 30 | previous head will always miss fids added by a some other |
| 31 | potential request |
| 32 | |
| 33 | All these problems could be avoided if fids were being added at the |
| 34 | end of the list. This can be achieved with a QSIMPLEQ, but this is |
| 35 | probably too much change for a bug fix. For now let's keep it |
| 36 | simple and just restart the loop from the current head. |
| 37 | |
| 38 | Fixes: CVE-2021-20181 |
| 39 | Buglink: https://bugs.launchpad.net/qemu/+bug/1911666 |
| 40 | Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com> |
| 41 | Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> |
| 42 | Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> |
| 43 | Message-Id: <161064025265.1838153.15185571283519390907.stgit@bahia.lan> |
| 44 | Signed-off-by: Greg Kurz <groug@kaod.org> |
| 45 | |
| 46 | Upstream-Status: Backport [89fbea8737e8f7b954745a1ffc4238d377055305] |
| 47 | CVE: CVE-2021-20181 |
| 48 | |
| 49 | Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com> |
| 50 | --- |
| 51 | hw/9pfs/9p.c | 6 +++--- |
| 52 | 1 file changed, 3 insertions(+), 3 deletions(-) |
| 53 | |
| 54 | diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c |
| 55 | index 94df440fc..6026b51a1 100644 |
| 56 | --- a/hw/9pfs/9p.c |
| 57 | +++ b/hw/9pfs/9p.c |
| 58 | @@ -502,9 +502,9 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) |
| 59 | { |
| 60 | int err; |
| 61 | V9fsState *s = pdu->s; |
| 62 | - V9fsFidState *fidp, head_fid; |
| 63 | + V9fsFidState *fidp; |
| 64 | |
| 65 | - head_fid.next = s->fid_list; |
| 66 | +again: |
| 67 | for (fidp = s->fid_list; fidp; fidp = fidp->next) { |
| 68 | if (fidp->path.size != path->size) { |
| 69 | continue; |
| 70 | @@ -524,7 +524,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) |
| 71 | * switched to the worker thread |
| 72 | */ |
| 73 | if (err == 0) { |
| 74 | - fidp = &head_fid; |
| 75 | + goto again; |
| 76 | } |
| 77 | } |
| 78 | } |
| 79 | -- |
| 80 | 2.29.2 |
| 81 | |