Re: GPF in shm_lock ipc

[Kirill A. Shutemov]

On Mon, Oct 12, 2015 at 08:18:21PM -0700, Davidlohr Bueso wrote:
> On Mon, 12 Oct 2015, Bueso wrote:
> 
> >On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
> >
> >>On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
> >>>diff --git a/ipc/shm.c b/ipc/shm.c
> >>>index 4178727..9615f19 100644
> >>>--- a/ipc/shm.c
> >>>+++ b/ipc/shm.c
> >>>@@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
> >>>static int shm_mmap(struct file *file, struct vm_area_struct *vma)
> >>>{
> >>>-	struct shm_file_data *sfd = shm_file_data(file);
> >>>+	struct file *vma_file = vma->vm_file;
> >>>+	struct shm_file_data *sfd = shm_file_data(vma_file);
> >>>+	struct ipc_ids *ids = &shm_ids(sfd->ns);
> >>>+	struct kern_ipc_perm *shp;
> >>>	int ret;
> >>>+	rcu_read_lock();
> >>>+	shp = ipc_obtain_object_check(ids, sfd->id);
> >>>+	if (IS_ERR(shp)) {
> >>>+		ret = -EINVAL;
> >>>+		goto err;
> >>>+	}
> >>>+
> >>>+	if (!ipc_valid_object(shp)) {
> >>>+		ret = -EIDRM;
> >>>+		goto err;
> >>>+	}
> >>>+	rcu_read_unlock();
> >>>+
> >>
> >>Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?
> >
> >Nothing, but that is later caught by shm_open() doing similar checks. We
> >basically end up doing a check between ->mmap() calls, which is fair imho.
> >Note that this can occur anywhere in ipc as IPC_RMID is a user request/cmd,
> >and we try to respect it -- thus you can argue this race anywhere, which is
> >why we have EIDRM/EINVL. Ultimately the user should not be doing such hacks
> >_anyway_. So I'm not really concerned about it.
> >
> >Another similar alternative would be perhaps to make shm_lock() return an
> >error, and thus propagate that error to mmap return. That way we would have
> >a silent way out of the warning scenario (afterward we cannot race as we
> >hold the ipc object lock). However, the users would now have to take this
> >into account...
> >
> >    [validity check lockless]
> >    ->mmap()
> >    [validity check lock]
> 
> Something like this, maybe. Although I could easily be missing things...
> I've tested it enough to see Dimitry's testcase handled ok, and put it
> through ltp. Also adding Manfred to the Cc, who always catches my idiotic
> mistakes.
> 
> 8<---------------------------------------------------------------------
> From: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> Date: Mon, 12 Oct 2015 19:38:34 -0700
> Subject: [PATCH] ipc/shm: fix handling of (re)attaching to a deleted segment
> 
> There are currently two issues when dealing with segments that are
> marked for deletion:
> 
> (i) With d0edd8528362 (ipc: convert invalid scenarios to use WARN_ON)
> we relaxed the system-wide impact of using a deleted segment. However,
> we can now perfectly well trigger the warning and then deference a nil
> pointer -- where shp does not exist.
> 
> (ii) As of a399b29dfbaa (ipc,shm: fix shm_file deletion races) we
> forbid attaching/mapping a previously deleted segment; a feature once
> unique to Linux, but removed[1] as a side effect of lockless ipc object
> lookups and security checks. Similarly, Dmitry Vyukov reported[2] a
> simple test case that creates a new vma for a previously deleted
> segment, triggering the WARN_ON mentioned in (i).
> 
> This patch tries to address (i) by moving the shp error check out
> of shm_lock() and handled by the caller instead. The benefit of this
> is that it allows better handling out of situations where we end up
> returning ERMID or EINVAL. Specifically, there are three callers
> of shm_lock which we must look into:
> 
>  - open/close -- which we ensure to never do any operations on
>                  the pairs, thus becoming no-ops if found a prev
> 		 IPC_RMID.
> 
>  - loosing the reference of nattch upon shmat(2) -- not feasible.
> 
> In addition, the common WARN_ON call is technically removed, but
> we add a new one for the bogus shmat(2) case, which is definitely
> unacceptable to race with RMID if nattch is bumped up.
> 
> To address (ii), a new shm_check_vma_validity() helper is added
> (for lack of a better name), which attempts to detect early on
> any races with RMID, before doing the full ->mmap. There is still
> a window between the callback and the shm_open call where we can
> race with IPC_RMID. If this is the case, it is handled by the next
> shm_lock().
> 
> shm_mmap:
>     [shm validity checks lockless]
>     ->mmap()
>     [shm validity checks lock] <-- at this point there after there
>                                    is no race as we hold the ipc
>                                    object lock.
> 
> [1] https://lkml.org/lkml/2015/10/12/483
> [2] https://lkml.org/lkml/2015/10/12/284
> 
> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
> ---
>  ipc/shm.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 4178727..47a7a67 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -156,11 +156,10 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
>  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
>  	/*
> -	 * We raced in the idr lookup or with shm_destroy().  Either way, the
> -	 * ID is busted.
> +	 * Callers of shm_lock() must validate the status of the returned
> +	 * ipc object pointer (as returned by ipc_lock()), and error out as
> +	 * appropriate.
>  	 */
> -	WARN_ON(IS_ERR(ipcp));
> -
>  	return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
> @@ -194,6 +193,15 @@ static void shm_open(struct vm_area_struct *vma)
>  	struct shmid_kernel *shp;
>  	shp = shm_lock(sfd->ns, sfd->id);
> +	/*
> +	 * We raced in the idr lookup or with shm_destroy().
> +	 * Either way, the ID is busted. In the same scenario,
> +	 * but for the close counter-part, the nattch counter
> +	 * is never decreased, thus we can safely return.
> +	 */
> +	if (IS_ERR(shp))
> +		return; /* no-op */
> +
>  	shp->shm_atim = get_seconds();
>  	shp->shm_lprid = task_tgid_vnr(current);
>  	shp->shm_nattch++;

...

>  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct shm_file_data *sfd = shm_file_data(file);
>  	int ret;
> +	/*
> +	 * Ensure that we have not raced with IPC_RMID, such that
> +	 * we avoid doing the ->mmap altogether. This is a preventive
> +	 * lockless check, and thus exposed to races during the mmap.
> +	 * However, this is later caught in shm_open(), and handled
> +	 * accordingly.
> +	 */
> +	ret = shm_check_vma_validity(vma);
> +	if (ret)
> +		return ret;
> +
>  	ret = sfd->file->f_op->mmap(sfd->file, vma);
>  	if (ret != 0)
>  		return ret;
> +
>  	sfd->vm_ops = vma->vm_ops;
>  #ifdef CONFIG_MMU
>  	WARN_ON(!sfd->vm_ops->fault);

If I read it correctly, with the patch we would ignore locking failure
inside shm_open() and mmap will succeed in this case. So the idea is to
have shm_close() no-op and therefore symmetrical. That's look fragile to
me. We would silently miss some other broken open/close pattern.

I would rather propagate error to shm_mmap() caller and therefore to
userspace. I guess it's better to opencode shm_open() in shm_mmap() and
return error this way. shm_open() itself can have WARN_ON_ONCE() for
failure or something.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/