Re: [PATCH] 9pfs: Fully restart unreclaim loop (CVE-2021-20181)

[Stefano Stabellini]

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-disclosures@trendmicro.com>
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Stefano Stabellini <sstabellini@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;
>              }
>          }
>      }
> 
>