linux-kernel - Re: io_wq_work task

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] [day] [month] [year] [list]

Date:   Fri, 12 Jun 2020 22:19:05 +0300
From:   Pavel Begunkov <asml.silence@...il.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jens Axboe <axboe@...nel.dk>
Cc:     linux-kernel@...r.kernel.org, io-uring@...r.kernel.org
Subject: Re: io_wq_work task_pid is nonsense

On 04/06/2020 14:39, Eric W. Biederman wrote:
> I was looking at something else and I happened to come across the
> task_pid field in struct io_wq_work.  The field is initialized with
> task_pid_vnr.  Then it is used for cancelling pending work.
> 
> The only appropriate and safe use of task_pid_vnr is for sending
> a pid value to userspace and that is not what is going on here.
> 
> This use is particularly bad as it looks like I can start a pid
> namespace create an io work queue and create threads that happen to have
> the userspace pid in question and then terminate them, or close their
> io_work_queue file descriptors, and wind up closing someone else's work.
> 
> There is also pid wrap around, and the craziness of de_thread to contend
> with as well.

Thanks reporting about this. It's not as bad because it's limited to a
single io_uring instance and is more like precautions. False positives
should be handled in userspace.

> Perhaps since all the task_pid field is used for is cancelling work for
> an individual task you could do something like the patch below.  I am
> assuming no reference counting is necessary as the field can not live
> past the life of a task.
> 
> Of cource the fact that you don't perform this work for file descriptors
> that are closed just before a task exits makes me wonder.
> 
> Can you please fix this code up to do something sensible?
> Maybe like below?

I'll deal with it. Needs a bit extra to not screw work->task refcounting,
but the idea looks right.

> 
> Eric
> 
> diff --git a/fs/io-wq.h b/fs/io-wq.h
> index 5ba12de7572f..bef29fff7403 100644
> --- a/fs/io-wq.h
> +++ b/fs/io-wq.h
> @@ -91,7 +91,7 @@ struct io_wq_work {
>  	const struct cred *creds;
>  	struct fs_struct *fs;
>  	unsigned flags;
> -	pid_t task_pid;
> +	struct task_struct *task;
>  };
>  
>  #define INIT_IO_WORK(work, _func)				\
> @@ -129,7 +129,7 @@ static inline bool io_wq_is_hashed(struct io_wq_work *work)
>  
>  void io_wq_cancel_all(struct io_wq *wq);
>  enum io_wq_cancel io_wq_cancel_work(struct io_wq *wq, struct io_wq_work *cwork);
> -enum io_wq_cancel io_wq_cancel_pid(struct io_wq *wq, pid_t pid);
> +enum io_wq_cancel io_wq_cancel_task(struct io_wq *wq, struct task_struct *task);
>  
>  typedef bool (work_cancel_fn)(struct io_wq_work *, void *);
>  
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 4023c9846860..2139a049d548 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -1004,18 +1004,16 @@ enum io_wq_cancel io_wq_cancel_work(struct io_wq *wq, struct io_wq_work *cwork)
>  	return io_wq_cancel_cb(wq, io_wq_io_cb_cancel_data, (void *)cwork);
>  }
>  
> -static bool io_wq_pid_match(struct io_wq_work *work, void *data)
> +static bool io_wq_task_match(struct io_wq_work *work, void *data)
>  {
> -	pid_t pid = (pid_t) (unsigned long) data;
> +	struct task_struct *task = data;
>  
> -	return work->task_pid == pid;
> +	return work->task == task;
>  }
>  
> -enum io_wq_cancel io_wq_cancel_pid(struct io_wq *wq, pid_t pid)
> +enum io_wq_cancel io_wq_cancel_task(struct io_wq *wq, struct task_struct *task)
>  {
> -	void *data = (void *) (unsigned long) pid;
> -
> -	return io_wq_cancel_cb(wq, io_wq_pid_match, data);
> +	return io_wq_cancel_cb(wq, io_wq_task_match, task);
>  }
>  
>  struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data)
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c687f57fb651..b9d557a21a26 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1031,8 +1031,8 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
>  		}
>  		spin_unlock(&current->fs->lock);
>  	}
> -	if (!req->work.task_pid)
> -		req->work.task_pid = task_pid_vnr(current);
> +	if (!req->work.task)
> +		req->work.task = current;
>  }
>  
>  static inline void io_req_work_drop_env(struct io_kiocb *req)
> @@ -7421,7 +7421,7 @@ static int io_uring_flush(struct file *file, void *data)
>  	 * If the task is going away, cancel work it may have pending
>  	 */
>  	if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
> -		io_wq_cancel_pid(ctx->io_wq, task_pid_vnr(current));
> +		io_wq_cancel_task(ctx->io_wq, current);
>  
>  	return 0;
>  }
> 

-- 
Pavel Begunkov