linux-kernel - Re: [PATCH] net/9p/trans_fd.c: fix double list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180723030251.GB24608@nautica>
Date:   Mon, 23 Jul 2018 05:02:51 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Tomas Bortoli <tomasbortoli@...il.com>
Cc:     ericvh@...il.com, rminnich@...dia.gov, lucho@...kov.net,
        jiangyiwen@...wei.com, davem@...emloft.net,
        v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: [PATCH] net/9p/trans_fd.c: fix double list_del() and race in
 access

Tomas Bortoli wrote on Fri, Jul 20, 2018:
> This patch uses list_del_init() instead of list_del() to eliminate
> "req_list". This to prevent double list_del()'s calls to the same list
> from provoking a GPF. Furthermore, this patch fixes an access to
> "req_list" that was made without getting the relative lock.

Please see comment about locking.

As for list_del to list_del_init, it feels a little wrong to me, but I
don't have a better idea so let's go with that.
Do you know what happened to trigger this? one thread running
p9_conn_cancel then the other thread doing p9_fd_cancel ?

> Signed-off-by: Tomas Bortoli <tomasbortoli@...il.com>
> Reported-by: syzbot+735d926e9d1317c3310c@...kaller.appspotmail.com
> ---
> 
>  net/9p/trans_fd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index a64b01c56e30..131bb1f059e6 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -223,7 +223,9 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
>  
>  	list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
>  		p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
> -		list_del(&req->req_list);
> +		spin_lock_irqsave(&m->client->lock, flags);
> +		list_del_init(&req->req_list);
> +		spin_unlock_irqrestore(&m->client->lock, flags);

Just locking around one item if you're afraid it might change won't be
enough - list_for_each_entry_safe is only "safe" from removing the
current element from the list yourself, not from other threads messing
with it, so you'd need to lock around the whole loop if that's what
you're protecting against.

(Also, since I've taken the other patchs to change spin locks on
client->lock to spin_lock instead of spin_lock_irqsave, please use that
function for new locking of that variable - in general just basing your
patchs off linux-next's master branch is a good idea.)

>  		if (!req->t_err)
>  			req->t_err = err;
>  		p9_client_cb(m->client, req, REQ_STATUS_ERROR);
> @@ -369,7 +371,7 @@ static void p9_read_work(struct work_struct *work)
>  		spin_lock(&m->client->lock);
>  		if (m->req->status != REQ_STATUS_ERROR)
>  			status = REQ_STATUS_RCVD;
> -		list_del(&m->req->req_list);
> +		list_del_init(&m->req->req_list);
>  		spin_unlock(&m->client->lock);
>  		p9_client_cb(m->client, m->req, status);
>  		m->rc.sdata = NULL;
> @@ -684,7 +686,7 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
>  	spin_lock(&client->lock);
>  
>  	if (req->status == REQ_STATUS_UNSENT) {
> -		list_del(&req->req_list);
> +		list_del_init(&req->req_list);
>  		req->status = REQ_STATUS_FLSHD;
>  		ret = 0;
>  	}
> @@ -701,7 +703,7 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
>  	 * remove it from the list.
>  	 */
>  	spin_lock(&client->lock);
> -	list_del(&req->req_list);
> +	list_del_init(&req->req_list);
>  	spin_unlock(&client->lock);
>  
>  	return 0;