On Thu, 14 Jan 2021, Greg Kurz wrote: > Depending on the client activity, the server can be asked to open a huge > number of file descriptors and eventually hit RLIMIT_NOFILE. This is > currently mitigated using a reclaim logic : the server closes the file > descriptors of idle fids, based on the assumption that it will be able > to re-open them later. This assumption doesn't hold of course if the > client requests the file to be unlinked. In this case, we loop on the > entire fid list and mark all related fids as unreclaimable (the reclaim > logic will just ignore them) and, of course, we open or re-open their > file descriptors if needed since we're about to unlink the file. > > This is the purpose of v9fs_mark_fids_unreclaim(). Since the actual > opening of a file can cause the coroutine to yield, another client > request could possibly add a new fid that we may want to mark as > non-reclaimable as well. The loop is thus restarted if the re-open > request was actually transmitted to the backend. This is achieved > by keeping a reference on the first fid (head) before traversing > the list. > > This is wrong in several ways: > - a potential clunk request from the client could tear the first > fid down and cause the reference to be stale. This leads to a > use-after-free error that can be detected with ASAN, using a > custom 9p client > - fids are added at the head of the list : restarting from the > previous head will always miss fids added by a some other > potential request > > All these problems could be avoided if fids were being added at the > end of the list. This can be achieved with a QSIMPLEQ, but this is > probably too much change for a bug fix. For now let's keep it > simple and just restart the loop from the current head. > > Fixes: CVE-2021-20181 > Buglink: https://bugs.launchpad.net/qemu/+bug/1911666 > Reported-by: Zero Day Initiative <zdi-disclosu...@trendmicro.com> > Reviewed-by: Christian Schoenebeck <qemu_...@crudebyte.com> > Signed-off-by: Greg Kurz <gr...@kaod.org>
Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> > --- > hw/9pfs/9p.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 94df440fc740..6026b51a1c04 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -502,9 +502,9 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU > *pdu, V9fsPath *path) > { > int err; > V9fsState *s = pdu->s; > - V9fsFidState *fidp, head_fid; > + V9fsFidState *fidp; > > - head_fid.next = s->fid_list; > +again: > for (fidp = s->fid_list; fidp; fidp = fidp->next) { > if (fidp->path.size != path->size) { > continue; > @@ -524,7 +524,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU > *pdu, V9fsPath *path) > * switched to the worker thread > */ > if (err == 0) { > - fidp = &head_fid; > + goto again; > } > } > } > >