Re: GPF in shm_lock ipc

2016-02-02 Thread Dmitry Vyukov 
On Tue, Feb 2, 2016 at 4:25 AM, Andrew Morton  wrote:
> On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov  wrote:
>
>> On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
>>  wrote:
>> > What about this:
>>
>>
>> Ping. This is still happening for me on tip. Can we pull in this fix
>> if it looks good to everybody?
>>
>
> Well we have at least three patches to choose from in this thread, most
> of them missing most signs of having been reviewed or tested.
>
> So I grabbed the last one, below.  Can we please rev this up again,
> test it, see if we can agree that this is the way to go forward?


Well, I can say that this patch fixes the original reproducer for me.
I will restart the fuzzer with it.


> From: "Kirill A. Shutemov" 
> Subject: ipc/shm: handle removed segments gracefully in shm_mmap()
>
> remap_file_pages(2) emulation can reach file which represents removed IPC
> ID as long as a memory segment is mapped.  It breaks expectations of IPC
> subsystem.
>
> Test case (rewritten to be more human readable, originally autogenerated
> by syzkaller[1]):
>
> #define _GNU_SOURCE
> #include 
> #include 
> #include 
> #include 
>
> #define PAGE_SIZE 4096
>
> int main()
> {
> int id;
> void *p;
>
> id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> p = shmat(id, NULL, 0);
> shmctl(id, IPC_RMID, NULL);
> remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
> return 0;
> }
>
> The patch changes shm_mmap() and code around shm_lock() to propagate
> locking error back to caller of shm_mmap().
>
> [1] http://github.com/google/syzkaller
>
> Signed-off-by: Kirill A. Shutemov 
> Reported-by: Dmitry Vyukov 
> Cc: Davidlohr Bueso 
> Cc: Manfred Spraul 
> Cc: 
> Signed-off-by: Andrew Morton 
> ---
>
>  ipc/shm.c |   53 ++--
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff -puN ipc/shm.c~gpf-in-shm_lock-ipc ipc/shm.c
> --- a/ipc/shm.c~gpf-in-shm_lock-ipc
> +++ a/ipc/shm.c
> @@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_l
> struct kern_ipc_perm *ipcp = ipc_lock(_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));
> -
> +   if (IS_ERR(ipcp))
> +   return (void *)ipcp;
> return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
>
> @@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_n
>  }
>
>
> -/* This is called by fork, once for every shm attach. */
> -static void shm_open(struct vm_area_struct *vma)
> +static int __shm_open(struct vm_area_struct *vma)
>  {
> struct file *file = vma->vm_file;
> struct shm_file_data *sfd = shm_file_data(file);
> struct shmid_kernel *shp;
>
> shp = shm_lock(sfd->ns, sfd->id);
> +
> +   if (IS_ERR(shp))
> +   return PTR_ERR(shp);
> +
> shp->shm_atim = get_seconds();
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_nattch++;
> shm_unlock(shp);
> +   return 0;
> +}
> +
> +/* This is called by fork, once for every shm attach. */
> +static void shm_open(struct vm_area_struct *vma)
> +{
> +   int err = __shm_open(vma);
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   WARN_ON_ONCE(err);
>  }
>
>  /*
> @@ -260,6 +276,14 @@ static void shm_close(struct vm_area_str
> down_write(_ids(ns).rwsem);
> /* remove from the list of attaches of the shm segment */
> shp = shm_lock(ns, sfd->id);
> +
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   if (WARN_ON_ONCE(IS_ERR(shp)))
> +   goto done; /* no-op */
> +
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_dtim = get_seconds();
> shp->shm_nattch--;
> @@ -267,6 +291,7 @@ static void shm_close(struct vm_area_str
> shm_destroy(ns, shp);
> else
> shm_unlock(shp);
> +done:
> up_write(_ids(ns).rwsem)

Re: GPF in shm_lock ipc

2016-02-02 Thread Dmitry Vyukov 
On Tue, Feb 2, 2016 at 4:25 AM, Andrew Morton <a...@linux-foundation.org> wrote:
> On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov <dvyu...@google.com> wrote:
>
>> On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
>> <kirill.shute...@linux.intel.com> wrote:
>> > What about this:
>>
>>
>> Ping. This is still happening for me on tip. Can we pull in this fix
>> if it looks good to everybody?
>>
>
> Well we have at least three patches to choose from in this thread, most
> of them missing most signs of having been reviewed or tested.
>
> So I grabbed the last one, below.  Can we please rev this up again,
> test it, see if we can agree that this is the way to go forward?


Well, I can say that this patch fixes the original reproducer for me.
I will restart the fuzzer with it.


> From: "Kirill A. Shutemov" <kirill.shute...@linux.intel.com>
> Subject: ipc/shm: handle removed segments gracefully in shm_mmap()
>
> remap_file_pages(2) emulation can reach file which represents removed IPC
> ID as long as a memory segment is mapped.  It breaks expectations of IPC
> subsystem.
>
> Test case (rewritten to be more human readable, originally autogenerated
> by syzkaller[1]):
>
> #define _GNU_SOURCE
> #include 
> #include 
> #include 
> #include 
>
> #define PAGE_SIZE 4096
>
> int main()
> {
> int id;
> void *p;
>
> id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> p = shmat(id, NULL, 0);
> shmctl(id, IPC_RMID, NULL);
> remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
> return 0;
> }
>
> The patch changes shm_mmap() and code around shm_lock() to propagate
> locking error back to caller of shm_mmap().
>
> [1] http://github.com/google/syzkaller
>
> Signed-off-by: Kirill A. Shutemov <kirill.shute...@linux.intel.com>
> Reported-by: Dmitry Vyukov <dvyu...@google.com>
> Cc: Davidlohr Bueso <d...@stgolabs.net>
> Cc: Manfred Spraul <manf...@colorfullife.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Andrew Morton <a...@linux-foundation.org>
> ---
>
>  ipc/shm.c |   53 ++--
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff -puN ipc/shm.c~gpf-in-shm_lock-ipc ipc/shm.c
> --- a/ipc/shm.c~gpf-in-shm_lock-ipc
> +++ a/ipc/shm.c
> @@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_l
> struct kern_ipc_perm *ipcp = ipc_lock(_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));
> -
> +   if (IS_ERR(ipcp))
> +   return (void *)ipcp;
> return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
>
> @@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_n
>  }
>
>
> -/* This is called by fork, once for every shm attach. */
> -static void shm_open(struct vm_area_struct *vma)
> +static int __shm_open(struct vm_area_struct *vma)
>  {
> struct file *file = vma->vm_file;
> struct shm_file_data *sfd = shm_file_data(file);
> struct shmid_kernel *shp;
>
> shp = shm_lock(sfd->ns, sfd->id);
> +
> +   if (IS_ERR(shp))
> +   return PTR_ERR(shp);
> +
> shp->shm_atim = get_seconds();
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_nattch++;
> shm_unlock(shp);
> +   return 0;
> +}
> +
> +/* This is called by fork, once for every shm attach. */
> +static void shm_open(struct vm_area_struct *vma)
> +{
> +   int err = __shm_open(vma);
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   WARN_ON_ONCE(err);
>  }
>
>  /*
> @@ -260,6 +276,14 @@ static void shm_close(struct vm_area_str
> down_write(_ids(ns).rwsem);
> /* remove from the list of attaches of the shm segment */
> shp = shm_lock(ns, sfd->id);
> +
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   if (WARN_ON_ONCE(IS_ERR(shp)))
> +   goto done; /* no-op */
> +
> shp->shm_lprid = task_t

Re: GPF in shm_lock ipc

2016-02-01 Thread Andrew Morton 
On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov  wrote:

> On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
>  wrote:
> > What about this:
> 
> 
> Ping. This is still happening for me on tip. Can we pull in this fix
> if it looks good to everybody?
> 

Well we have at least three patches to choose from in this thread, most
of them missing most signs of having been reviewed or tested.

So I grabbed the last one, below.  Can we please rev this up again,
test it, see if we can agree that this is the way to go forward?

Thanks.



From: "Kirill A. Shutemov" 
Subject: ipc/shm: handle removed segments gracefully in shm_mmap()

remap_file_pages(2) emulation can reach file which represents removed IPC
ID as long as a memory segment is mapped.  It breaks expectations of IPC
subsystem.

Test case (rewritten to be more human readable, originally autogenerated
by syzkaller[1]):

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

return 0;
}

The patch changes shm_mmap() and code around shm_lock() to propagate
locking error back to caller of shm_mmap().

[1] http://github.com/google/syzkaller

Signed-off-by: Kirill A. Shutemov 
Reported-by: Dmitry Vyukov 
Cc: Davidlohr Bueso 
Cc: Manfred Spraul 
Cc: 
Signed-off-by: Andrew Morton 
---

 ipc/shm.c |   53 ++--
 1 file changed, 43 insertions(+), 10 deletions(-)

diff -puN ipc/shm.c~gpf-in-shm_lock-ipc ipc/shm.c
--- a/ipc/shm.c~gpf-in-shm_lock-ipc
+++ a/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_l
struct kern_ipc_perm *ipcp = ipc_lock(_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));
-
+   if (IS_ERR(ipcp))
+   return (void *)ipcp;
return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_n
 }
 
 
-/* This is called by fork, once for every shm attach. */
-static void shm_open(struct vm_area_struct *vma)
+static int __shm_open(struct vm_area_struct *vma)
 {
struct file *file = vma->vm_file;
struct shm_file_data *sfd = shm_file_data(file);
struct shmid_kernel *shp;
 
shp = shm_lock(sfd->ns, sfd->id);
+
+   if (IS_ERR(shp))
+   return PTR_ERR(shp);
+
shp->shm_atim = get_seconds();
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_nattch++;
shm_unlock(shp);
+   return 0;
+}
+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
 }
 
 /*
@@ -260,6 +276,14 @@ static void shm_close(struct vm_area_str
down_write(_ids(ns).rwsem);
/* remove from the list of attaches of the shm segment */
shp = shm_lock(ns, sfd->id);
+
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   if (WARN_ON_ONCE(IS_ERR(shp)))
+   goto done; /* no-op */
+
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_dtim = get_seconds();
shp->shm_nattch--;
@@ -267,6 +291,7 @@ static void shm_close(struct vm_area_str
shm_destroy(ns, shp);
else
shm_unlock(shp);
+done:
up_write(_ids(ns).rwsem);
 }
 
@@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, s
struct shm_file_data *sfd = shm_file_data(file);
int ret;
 
+   /*
+* In case of remap_file_pages() emulation, the file can represent
+* removed IPC ID: propogate shm_lock() error to caller.
+*/
+   ret =__shm_open(vma);
+   if (ret)
+   return ret;
+
ret = sfd->file->f_op->mmap(sfd->file, vma);
-   if (ret != 0)
+   if (ret) {
+   shm_close(vma);
return ret;
+   }
sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
WARN_ON(!sfd->vm_ops->fault);
 #endif
vma->vm_ops = _vm_ops;
-   shm_open(vma);
-
-   return ret;
+   return 0;
 }
 
 static int shm_release(struct inode *ino, struct file *file)
_

Re: GPF in shm_lock ipc

2016-02-01 Thread Andrew Morton 
On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov <dvyu...@google.com> wrote:

> On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
> <kirill.shute...@linux.intel.com> wrote:
> > What about this:
> 
> 
> Ping. This is still happening for me on tip. Can we pull in this fix
> if it looks good to everybody?
> 

Well we have at least three patches to choose from in this thread, most
of them missing most signs of having been reviewed or tested.

So I grabbed the last one, below.  Can we please rev this up again,
test it, see if we can agree that this is the way to go forward?

Thanks.



From: "Kirill A. Shutemov" <kirill.shute...@linux.intel.com>
Subject: ipc/shm: handle removed segments gracefully in shm_mmap()

remap_file_pages(2) emulation can reach file which represents removed IPC
ID as long as a memory segment is mapped.  It breaks expectations of IPC
subsystem.

Test case (rewritten to be more human readable, originally autogenerated
by syzkaller[1]):

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

return 0;
}

The patch changes shm_mmap() and code around shm_lock() to propagate
locking error back to caller of shm_mmap().

[1] http://github.com/google/syzkaller

Signed-off-by: Kirill A. Shutemov <kirill.shute...@linux.intel.com>
Reported-by: Dmitry Vyukov <dvyu...@google.com>
Cc: Davidlohr Bueso <d...@stgolabs.net>
Cc: Manfred Spraul <manf...@colorfullife.com>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Andrew Morton <a...@linux-foundation.org>
---

 ipc/shm.c |   53 ++++++++++------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff -puN ipc/shm.c~gpf-in-shm_lock-ipc ipc/shm.c
--- a/ipc/shm.c~gpf-in-shm_lock-ipc
+++ a/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_l
struct kern_ipc_perm *ipcp = ipc_lock(_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));
-
+   if (IS_ERR(ipcp))
+   return (void *)ipcp;
return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_n
 }
 
 
-/* This is called by fork, once for every shm attach. */
-static void shm_open(struct vm_area_struct *vma)
+static int __shm_open(struct vm_area_struct *vma)
 {
struct file *file = vma->vm_file;
struct shm_file_data *sfd = shm_file_data(file);
struct shmid_kernel *shp;
 
shp = shm_lock(sfd->ns, sfd->id);
+
+   if (IS_ERR(shp))
+   return PTR_ERR(shp);
+
shp->shm_atim = get_seconds();
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_nattch++;
shm_unlock(shp);
+   return 0;
+}
+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
 }
 
 /*
@@ -260,6 +276,14 @@ static void shm_close(struct vm_area_str
down_write(_ids(ns).rwsem);
/* remove from the list of attaches of the shm segment */
shp = shm_lock(ns, sfd->id);
+
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   if (WARN_ON_ONCE(IS_ERR(shp)))
+   goto done; /* no-op */
+
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_dtim = get_seconds();
shp->shm_nattch--;
@@ -267,6 +291,7 @@ static void shm_close(struct vm_area_str
shm_destroy(ns, shp);
else
shm_unlock(shp);
+done:
up_write(_ids(ns).rwsem);
 }
 
@@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, s
struct shm_file_data *sfd = shm_file_data(file);
int ret;
 
+   /*
+* In case of remap_file_pages() emulation, the file can represent
+* removed IPC ID: propogate shm_lock() error to caller.
+*/
+   ret =__shm_open(vma);
+   if (ret)
+   return ret;
+
ret = sfd->file->f_op->mmap(sfd->file, vma);
-   if (ret != 0)
+   if (ret) {
+   shm_close(vma);

Re: GPF in shm_lock ipc

2016-01-02 Thread Manfred Spraul 

Hi Dmitry,

On 01/02/2016 01:19 PM, Dmitry Vyukov wrote:

On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
 wrote:

Hi Dmitry,

shm locking differs too much from msg/sem locking, I never looked at it in
depth, so I'm not able to perform a proper review.

Except for the obvious: Races that can be triggered from user space are
inacceptable.
Regardless if there is a BUG_ON, a WARN_ON or nothing at all.

On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:

+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
   }

Is it possible to trigger this race? Parallel IPC_RMID & fork()?

Hi Manfred,

As far as I see my reproducer triggers exactly this warning (and later a crash).

Do I understand it right, shm_open() is also called by remap()?
Then please update the comment above shm_open().

And: If this is something that userspace can trigger, why a WARN_ON_ONCE()?
If the WARN_ON doesn't indicate a bug, then I would remove it entirely.

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

Re: GPF in shm_lock ipc

2016-01-02 Thread Dmitry Vyukov 
On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
 wrote:
> Hi Dmitry,
>
> shm locking differs too much from msg/sem locking, I never looked at it in
> depth, so I'm not able to perform a proper review.
>
> Except for the obvious: Races that can be triggered from user space are
> inacceptable.
> Regardless if there is a BUG_ON, a WARN_ON or nothing at all.
>
> On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:
>>>
>>> +
>>> +/* This is called by fork, once for every shm attach. */
>>> +static void shm_open(struct vm_area_struct *vma)
>>> +{
>>> +   int err = __shm_open(vma);
>>> +   /*
>>> +* We raced in the idr lookup or with shm_destroy().
>>> +* Either way, the ID is busted.
>>> +*/
>>> +   WARN_ON_ONCE(err);
>>>   }
>
> Is it possible to trigger this race? Parallel IPC_RMID & fork()?

Hi Manfred,

As far as I see my reproducer triggers exactly this warning (and later a crash).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2016-01-02 Thread Manfred Spraul 

Hi Dmitry,

shm locking differs too much from msg/sem locking, I never looked at it 
in depth, so I'm not able to perform a proper review.


Except for the obvious: Races that can be triggered from user space are 
inacceptable.

Regardless if there is a BUG_ON, a WARN_ON or nothing at all.

On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:

+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
  }

Is it possible to trigger this race? Parallel IPC_RMID & fork()?

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

Re: GPF in shm_lock ipc

2016-01-02 Thread Manfred Spraul 

Hi Dmitry,

shm locking differs too much from msg/sem locking, I never looked at it 
in depth, so I'm not able to perform a proper review.


Except for the obvious: Races that can be triggered from user space are 
inacceptable.

Regardless if there is a BUG_ON, a WARN_ON or nothing at all.

On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:

+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
  }

Is it possible to trigger this race? Parallel IPC_RMID & fork()?

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

Re: GPF in shm_lock ipc

2016-01-02 Thread Dmitry Vyukov 
On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
 wrote:
> Hi Dmitry,
>
> shm locking differs too much from msg/sem locking, I never looked at it in
> depth, so I'm not able to perform a proper review.
>
> Except for the obvious: Races that can be triggered from user space are
> inacceptable.
> Regardless if there is a BUG_ON, a WARN_ON or nothing at all.
>
> On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:
>>>
>>> +
>>> +/* This is called by fork, once for every shm attach. */
>>> +static void shm_open(struct vm_area_struct *vma)
>>> +{
>>> +   int err = __shm_open(vma);
>>> +   /*
>>> +* We raced in the idr lookup or with shm_destroy().
>>> +* Either way, the ID is busted.
>>> +*/
>>> +   WARN_ON_ONCE(err);
>>>   }
>
> Is it possible to trigger this race? Parallel IPC_RMID & fork()?

Hi Manfred,

As far as I see my reproducer triggers exactly this warning (and later a crash).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2016-01-02 Thread Manfred Spraul 

Hi Dmitry,

On 01/02/2016 01:19 PM, Dmitry Vyukov wrote:

On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
 wrote:

Hi Dmitry,

shm locking differs too much from msg/sem locking, I never looked at it in
depth, so I'm not able to perform a proper review.

Except for the obvious: Races that can be triggered from user space are
inacceptable.
Regardless if there is a BUG_ON, a WARN_ON or nothing at all.

On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:

+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+   int err = __shm_open(vma);
+   /*
+* We raced in the idr lookup or with shm_destroy().
+* Either way, the ID is busted.
+*/
+   WARN_ON_ONCE(err);
   }

Is it possible to trigger this race? Parallel IPC_RMID & fork()?

Hi Manfred,

As far as I see my reproducer triggers exactly this warning (and later a crash).

Do I understand it right, shm_open() is also called by remap()?
Then please update the comment above shm_open().

And: If this is something that userspace can trigger, why a WARN_ON_ONCE()?
If the WARN_ON doesn't indicate a bug, then I would remove it entirely.

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

Re: GPF in shm_lock ipc

2015-12-21 Thread Dmitry Vyukov 
On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
 wrote:
> What about this:


Ping. This is still happening for me on tip. Can we pull in this fix
if it looks good to everybody?


> From 06b0fc9d62592f6f3ad9f45cccf1f6a5b3113bdc Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" 
> Date: Thu, 5 Nov 2015 15:53:04 +0200
> Subject: [PATCH] ipc/shm: handle removed segments gracefully in shm_mmap()
>
> remap_file_pages(2) emulation can reach file which represents removed
> IPC ID as long as a memory segment is mapped. It breaks expectations
> of IPC subsystem.
>
> Test case (rewritten to be more human readable, originally autogenerated
> by syzkaller[1]):
>
> #define _GNU_SOURCE
> #include 
> #include 
> #include 
> #include 
>
> #define PAGE_SIZE 4096
>
> int main()
> {
> int id;
> void *p;
>
> id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> p = shmat(id, NULL, 0);
> shmctl(id, IPC_RMID, NULL);
> remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
> return 0;
> }
>
> The patch changes shm_mmap() and code around shm_lock() to propagate
> locking error back to caller of shm_mmap().
>
> [1] http://github.com/google/syzkaller
>
> Signed-off-by: Kirill A. Shutemov 
> Reported-by: Dmitry Vyukov 
> Cc: Davidlohr Bueso 
> ---
>  ipc/shm.c | 53 +++--
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 41787276e141..3174634ca4e5 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct 
> ipc_namespace *ns, int id)
> struct kern_ipc_perm *ipcp = ipc_lock(_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));
> -
> +   if (IS_ERR(ipcp))
> +   return (void *)ipcp;
> return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
>
> @@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_namespace *ns, 
> struct shmid_kernel *s)
>  }
>
>
> -/* This is called by fork, once for every shm attach. */
> -static void shm_open(struct vm_area_struct *vma)
> +static int __shm_open(struct vm_area_struct *vma)
>  {
> struct file *file = vma->vm_file;
> struct shm_file_data *sfd = shm_file_data(file);
> struct shmid_kernel *shp;
>
> shp = shm_lock(sfd->ns, sfd->id);
> +
> +   if (IS_ERR(shp))
> +   return PTR_ERR(shp);
> +
> shp->shm_atim = get_seconds();
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_nattch++;
> shm_unlock(shp);
> +   return 0;
> +}
> +
> +/* This is called by fork, once for every shm attach. */
> +static void shm_open(struct vm_area_struct *vma)
> +{
> +   int err = __shm_open(vma);
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   WARN_ON_ONCE(err);
>  }
>
>  /*
> @@ -260,6 +276,14 @@ static void shm_close(struct vm_area_struct *vma)
> down_write(_ids(ns).rwsem);
> /* remove from the list of attaches of the shm segment */
> shp = shm_lock(ns, sfd->id);
> +
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   if (WARN_ON_ONCE(IS_ERR(shp)))
> +   goto done; /* no-op */
> +
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_dtim = get_seconds();
> shp->shm_nattch--;
> @@ -267,6 +291,7 @@ static void shm_close(struct vm_area_struct *vma)
> shm_destroy(ns, shp);
> else
> shm_unlock(shp);
> +done:
> up_write(_ids(ns).rwsem);
>  }
>
> @@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, struct 
> vm_area_struct *vma)
> struct shm_file_data *sfd = shm_file_data(file);
> int ret;
>
> +   /*
> +* In case of remap_file_pages() emulation, the file can represent
> +* removed IPC ID: propogate shm_lock() error to caller.
> +*/
> +   ret =__shm_open(vma);
> +   if (ret)
> +   return ret;
> +
> ret = sfd->file->f_op->mmap(sfd->file, vma);
> -   if (ret != 0)
> +   if (ret) {
> +   shm_close(vma);
> return ret;
> +   }
> sfd->vm_ops = vma->vm_ops;
>  #ifdef CONFIG_MMU
> WARN_ON(!sfd->vm_ops->fault);
>  #endif
> vma->vm_ops = _vm_ops;
> -   shm_open(vma);
> -
> -   return ret;
> +   return 0;
>  }
>
>  static int

Re: GPF in shm_lock ipc

2015-12-21 Thread Dmitry Vyukov 
On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
 wrote:
> What about this:


Ping. This is still happening for me on tip. Can we pull in this fix
if it looks good to everybody?


> From 06b0fc9d62592f6f3ad9f45cccf1f6a5b3113bdc Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" 
> Date: Thu, 5 Nov 2015 15:53:04 +0200
> Subject: [PATCH] ipc/shm: handle removed segments gracefully in shm_mmap()
>
> remap_file_pages(2) emulation can reach file which represents removed
> IPC ID as long as a memory segment is mapped. It breaks expectations
> of IPC subsystem.
>
> Test case (rewritten to be more human readable, originally autogenerated
> by syzkaller[1]):
>
> #define _GNU_SOURCE
> #include 
> #include 
> #include 
> #include 
>
> #define PAGE_SIZE 4096
>
> int main()
> {
> int id;
> void *p;
>
> id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> p = shmat(id, NULL, 0);
> shmctl(id, IPC_RMID, NULL);
> remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
> return 0;
> }
>
> The patch changes shm_mmap() and code around shm_lock() to propagate
> locking error back to caller of shm_mmap().
>
> [1] http://github.com/google/syzkaller
>
> Signed-off-by: Kirill A. Shutemov 
> Reported-by: Dmitry Vyukov 
> Cc: Davidlohr Bueso 
> ---
>  ipc/shm.c | 53 +++--
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 41787276e141..3174634ca4e5 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct 
> ipc_namespace *ns, int id)
> struct kern_ipc_perm *ipcp = ipc_lock(_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));
> -
> +   if (IS_ERR(ipcp))
> +   return (void *)ipcp;
> return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
>
> @@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_namespace *ns, 
> struct shmid_kernel *s)
>  }
>
>
> -/* This is called by fork, once for every shm attach. */
> -static void shm_open(struct vm_area_struct *vma)
> +static int __shm_open(struct vm_area_struct *vma)
>  {
> struct file *file = vma->vm_file;
> struct shm_file_data *sfd = shm_file_data(file);
> struct shmid_kernel *shp;
>
> shp = shm_lock(sfd->ns, sfd->id);
> +
> +   if (IS_ERR(shp))
> +   return PTR_ERR(shp);
> +
> shp->shm_atim = get_seconds();
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_nattch++;
> shm_unlock(shp);
> +   return 0;
> +}
> +
> +/* This is called by fork, once for every shm attach. */
> +static void shm_open(struct vm_area_struct *vma)
> +{
> +   int err = __shm_open(vma);
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   WARN_ON_ONCE(err);
>  }
>
>  /*
> @@ -260,6 +276,14 @@ static void shm_close(struct vm_area_struct *vma)
> down_write(_ids(ns).rwsem);
> /* remove from the list of attaches of the shm segment */
> shp = shm_lock(ns, sfd->id);
> +
> +   /*
> +* We raced in the idr lookup or with shm_destroy().
> +* Either way, the ID is busted.
> +*/
> +   if (WARN_ON_ONCE(IS_ERR(shp)))
> +   goto done; /* no-op */
> +
> shp->shm_lprid = task_tgid_vnr(current);
> shp->shm_dtim = get_seconds();
> shp->shm_nattch--;
> @@ -267,6 +291,7 @@ static void shm_close(struct vm_area_struct *vma)
> shm_destroy(ns, shp);
> else
> shm_unlock(shp);
> +done:
> up_write(_ids(ns).rwsem);
>  }
>
> @@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, struct 
> vm_area_struct *vma)
> struct shm_file_data *sfd = shm_file_data(file);
> int ret;
>
> +   /*
> +* In case of remap_file_pages() emulation, the file can represent
> +* removed IPC ID: propogate shm_lock() error to caller.
> +*/
> +   ret =__shm_open(vma);
> +   if (ret)
> +   return ret;
> +
> ret = sfd->file->f_op->mmap(sfd->file, vma);
> -   if (ret != 0)
> +   if (ret) {
> +   shm_close(vma);
> return ret;
> +   }
> sfd->vm_ops = vma->vm_ops;
>  #ifdef CONFIG_MMU
> WARN_ON(!sfd->vm_ops->fault);
>  #endif
>

Re: GPF in shm_lock ipc

2015-11-05 Thread Kirill A. Shutemov 
Dmitry Vyukov wrote:
> On Tue, Oct 13, 2015 at 8:30 PM, Kirill A. Shutemov
>  wrote:
> > 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 = _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 
> >> 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

Re: GPF in shm_lock ipc

2015-11-05 Thread Kirill A. Shutemov 
Dmitry Vyukov wrote:
> On Tue, Oct 13, 2015 at 8:30 PM, Kirill A. Shutemov
>  wrote:
> > 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 = _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 
> >> 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
> >>

Re: GPF in shm_lock ipc

2015-10-29 Thread Dmitry Vyukov 
On Tue, Oct 13, 2015 at 8:30 PM, Kirill A. Shutemov
 wrote:
> 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 = _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 
>> 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 
>> ---
>>  ipc/shm.c | 78 
>> +++
>>

Re: GPF in shm_lock ipc

2015-10-29 Thread Dmitry Vyukov 
On Tue, Oct 13, 2015 at 8:30 PM, Kirill A. Shutemov
 wrote:
> 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 = _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 
>> 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 
>> ---
>>  ipc/shm.c | 78 
>>

Re: GPF in shm_lock ipc

2015-10-13 Thread 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 = _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 
> 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 
> ---
>  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
> @@

Re: GPF in shm_lock ipc

2015-10-13 Thread 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 = _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 
> 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 
> ---
>  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
> ---

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

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 = _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 
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 
---
 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(_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

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

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 = _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]


Shouldn't we bump shm_nattch here? Or some other refcount?


At least not shm_nattach, as that would acknowledge a new attachment after
a valid IPC_RMID. But the problem is also with how we check for marked for
deletion segments -- ipc_valid_object() checking the deleted flag. As such,
we always rely on explicitly checking against the deleted flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2015-10-12 Thread Kirill A. Shutemov 
On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
> On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
> 
> >On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> >Here's slightly simplified and more human readable reproducer:
> >
> >#define _GNU_SOURCE
> >#include 
> >#include 
> >#include 
> >#include 
> >
> >#define PAGE_SIZE 4096
> >
> >int main()
> >{
> > int id;
> > void *p;
> >
> > id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> > p = shmat(id, NULL, 0);
> > shmctl(id, IPC_RMID, NULL);
> > remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
> >
> >   return 0;
> >}
> 
> Thanks!
> 
> >>
> >>On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> >>(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> >>
> >>[ cut here ]
> >>WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> >>Modules linked in:
> >>CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> >>Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> 81bcb43c 88081bf0bd70 812fe8d6 
> >> 88081bf0bda8 81051ff1 ffea 88081b896ca8
> >> 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
> >>Call Trace:
> >> [< inline >] __dump_stack lib/dump_stack.c:15
> >> [] dump_stack+0x44/0x5e lib/dump_stack.c:50
> >> [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
> >> [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
> >> [< inline >] shm_lock ipc/shm.c:162
> >> [] shm_open+0x74/0x80 ipc/shm.c:196
> >> [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
> >> [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
> >> [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
> >> [< inline >] do_mmap_pgoff include/linux/mm.h:1930
> >> [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
> >> [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
> >> [] entry_SYSCALL_64_fastpath+0x12/0x6a
> >>arch/x86/entry/entry_64.S:185
> >>---[ end trace 0873e743fc645a8c ]---
> >
> >Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
> >be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
> >VMA using existing one.
> 
> Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
> This is common to all things ipc, not only to shm. And while Linux nowadays
> does enforce that nothing touch a segment marked for deletion[1], we have
> contradictory scenarios where the resource is only freed once the last 
> attached
> process exits.
> 
> [1] https://lkml.org/lkml/2015/10/12/483
> 
> So this warning used to in fact be a full BUG_ON, but ultimately the ipc
> subsystem acknowledges that this situation is possible but fully blames the
> user responsible, and therefore we only warn about bogus usage.
> 
> >I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
> >normal from mm POV (no special flags, etc.) and it meats remap_file_pages
> >criteria (shared mapping). Every fix I can think of on mm side is ugly.
> >
> >Probably better to teach shm_mmap() to fall off gracefully in case of
> >non-existing shmid? I'm not familiar with IPC code.
> >Could anyone look into it?
> 
> Yeah, this was my approach as well. Very little tested other than it solves
> the above warning. Basically we don't want to be doing mmap if the segment
> was deleted, thus return a corresponding error instead of triggering the
> same error later on after mmaping, via shm_open(). I still need to think
> a bit more about this, but seems legit if we don't hurt userspace while
> at it (at least the idea, not considering any overhead in doing the idr
> lookup). Thoughts?
> 
> Thanks,
> Davidlohr
> 
> 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 = _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?
Shouldn't we bump shm_nattch here? Or some other refcount?


>   ret = sfd->file->f_op->mmap(sfd->file, vma);
>   if (ret != 0)
>   return ret;
> @@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct 
> vm_area_struct *vma)
>   shm_open(vma);
>   return ret;
> +err:
> + rcu_read_unlock();
> + return

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:


On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
Here's slightly simplified and more human readable reproducer:

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

   return 0;
}


Thanks!



On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 81bcb43c 88081bf0bd70 812fe8d6 
 88081bf0bda8 81051ff1 ffea 88081b896ca8
 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x44/0x5e lib/dump_stack.c:50
 [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
 [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
 [< inline >] shm_lock ipc/shm.c:162
 [] shm_open+0x74/0x80 ipc/shm.c:196
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
---[ end trace 0873e743fc645a8c ]---


Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
VMA using existing one.


Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
This is common to all things ipc, not only to shm. And while Linux nowadays
does enforce that nothing touch a segment marked for deletion[1], we have
contradictory scenarios where the resource is only freed once the last attached
process exits. 


[1] https://lkml.org/lkml/2015/10/12/483

So this warning used to in fact be a full BUG_ON, but ultimately the ipc
subsystem acknowledges that this situation is possible but fully blames the
user responsible, and therefore we only warn about bogus usage.


I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
normal from mm POV (no special flags, etc.) and it meats remap_file_pages
criteria (shared mapping). Every fix I can think of on mm side is ugly.

Probably better to teach shm_mmap() to fall off gracefully in case of
non-existing shmid? I'm not familiar with IPC code.
Could anyone look into it?


Yeah, this was my approach as well. Very little tested other than it solves
the above warning. Basically we don't want to be doing mmap if the segment
was deleted, thus return a corresponding error instead of triggering the
same error later on after mmaping, via shm_open(). I still need to think
a bit more about this, but seems legit if we don't hurt userspace while
at it (at least the idea, not considering any overhead in doing the idr
lookup). Thoughts?

Thanks,
Davidlohr

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 = _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();
+
ret = sfd->file->f_op->mmap(sfd->file, vma);
if (ret != 0)
return ret;
@@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct 
vm_area_struct *vma)
shm_open(vma);
 
 	return ret;

+err:
+   rcu_read_unlock();
+   return ret;
 }
 
 static int shm_release(struct inode *ino, struct file *file)







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

Re: GPF in shm_lock ipc

2015-10-12 Thread Kirill A. Shutemov 
On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> Hello,
> 
> The following program crashes kernel:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include 
> #include 
> #include 
> 
> int main()
> {
> long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
> long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
> long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
> 0x3000ul, 0x3ul, 0x207f9000ul);
> long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
> long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
> return 0;
> }

Here's slightly simplified and more human readable reproducer:

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

return 0;
}

> 
> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> 
> [ cut here ]
> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> Modules linked in:
> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  81bcb43c 88081bf0bd70 812fe8d6 
>  88081bf0bda8 81051ff1 ffea 88081b896ca8
>  880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
> Call Trace:
>  [< inline >] __dump_stack lib/dump_stack.c:15
>  [] dump_stack+0x44/0x5e lib/dump_stack.c:50
>  [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>  [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>  [< inline >] shm_lock ipc/shm.c:162
>  [] shm_open+0x74/0x80 ipc/shm.c:196
>  [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>  [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>  [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>  [< inline >] do_mmap_pgoff include/linux/mm.h:1930
>  [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
>  [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
>  [] entry_SYSCALL_64_fastpath+0x12/0x6a
> arch/x86/entry/entry_64.S:185
> ---[ end trace 0873e743fc645a8c ]---

Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
VMA using existing one.

I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
normal from mm POV (no special flags, etc.) and it meats remap_file_pages
criteria (shared mapping). Every fix I can think of on mm side is ugly.

Probably better to teach shm_mmap() to fall off gracefully in case of
non-existing shmid? I'm not familiar with IPC code.
Could anyone look into it?

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

Re: GPF in shm_lock ipc

2015-10-12 Thread Dmitry Vyukov 
On Mon, Oct 12, 2015 at 1:41 PM, Vlastimil Babka  wrote:
> On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:
>>
>> Hello,
>>
>> The following program crashes kernel:
>>
>> // autogenerated by syzkaller (http://github.com/google/syzkaller)
>> #include 
>> #include 
>> #include 
>>
>> int main()
>> {
>>  long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
>>  long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
>>  long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
>> 0x3000ul, 0x3ul, 0x207f9000ul);
>>  long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
>>  long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
>> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
>>  return 0;
>> }
>>
>> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
>> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
>>
>> [ cut here ]
>> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
>> Modules linked in:
>> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>> 01/01/2011
>>   81bcb43c 88081bf0bd70 812fe8d6 
>>   88081bf0bda8 81051ff1 ffea 88081b896ca8
>>   880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
>> Call Trace:
>>   [< inline >] __dump_stack lib/dump_stack.c:15
>>   [] dump_stack+0x44/0x5e lib/dump_stack.c:50
>>   [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>>   [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>>   [< inline >] shm_lock ipc/shm.c:162
>>   [] shm_open+0x74/0x80 ipc/shm.c:196
>>   [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>>   [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>>   [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>>   [< inline >] do_mmap_pgoff include/linux/mm.h:1930
>>   [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
>
>
> Hmm what kind of stack unwinder catches inlines? Some external patch, based
> on debuginfo?

We use the following script to symbolize kernel stack traces. It adds
file:line info and inlined frames.
https://github.com/google/sanitizers/blob/master/address-sanitizer/tools/kasan_symbolize.py
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2015-10-12 Thread Vlastimil Babka 

On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:

Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
 long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
 long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
 long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
0x3000ul, 0x3ul, 0x207f9000ul);
 long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
 long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
0x3000ul, 0x0ul, 0x7ul, 0x0ul);
 return 0;
}

On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  81bcb43c 88081bf0bd70 812fe8d6 
  88081bf0bda8 81051ff1 ffea 88081b896ca8
  880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
  [< inline >] __dump_stack lib/dump_stack.c:15
  [] dump_stack+0x44/0x5e lib/dump_stack.c:50
  [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
  [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
  [< inline >] shm_lock ipc/shm.c:162
  [] shm_open+0x74/0x80 ipc/shm.c:196
  [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
  [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
  [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
  [< inline >] do_mmap_pgoff include/linux/mm.h:1930
  [< inline >] SYSC_remap_file_pages mm/mmap.c:2694


Hmm what kind of stack unwinder catches inlines? Some external patch, 
based on debuginfo?


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

GPF in shm_lock ipc

2015-10-12 Thread Dmitry Vyukov 
Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
0x3000ul, 0x3ul, 0x207f9000ul);
long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
0x3000ul, 0x0ul, 0x7ul, 0x0ul);
return 0;
}

On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 81bcb43c 88081bf0bd70 812fe8d6 
 88081bf0bda8 81051ff1 ffea 88081b896ca8
 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x44/0x5e lib/dump_stack.c:50
 [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
 [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
 [< inline >] shm_lock ipc/shm.c:162
 [] shm_open+0x74/0x80 ipc/shm.c:196
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
---[ end trace 0873e743fc645a8c ]---
BUG: unable to handle kernel NULL pointer dereference at 003a
IP: [] shm_open+0x35/0x80 ipc/shm.c:197
PGD 81a08b067 PUD 81a01b067 PMD 0
Oops: 0002 [#1] SMP
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Tainted: GW   4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: 880819b26800 ti: 88081bf08000 task.ti: 88081bf08000
RIP: 0010:[]  [] shm_open+0x35/0x80
RSP: 0018:88081bf0bdc8  EFLAGS: 00010296
RAX: 560d1d13 RBX: ffea RCX: 8151e710
RDX: 0001 RSI:  RDI: 0009
RBP: 88081bf0bdd0 R08: 000a R09: 0001
R10:  R11: 01bc R12: 88081b896ca8
R13: 880819b81620 R14: 8800bbaa6d00 R15: 880819b81600
FS:  7fbfd966f700() GS:88083fd0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 003a CR3: 000819a97000 CR4: 06e0
Stack:
 880819b81200 88081bf0bdf8 81295cbe 19b81620
 880819b81628 207f9000 88081bf0be80 81142d14
 8800bbaa6d00 0007  0007
Call Trace:
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
Code: 00 00 48 8b 80 d0 00 00 00 48 8b 50 08 8b 30 48 8d ba c8 00 00
00 e8 6b b1 ff ff 48 3d 00 f0 ff ff 48 89 c3 77 33 e8 5b 6b e1 ff <48>
89 43 50 65 48 8b 04 25 c0 ad 00 00 48 8b 80 40 04 00 00 48
RIP  [] shm_open+0x35/0x80 ipc/shm.c:197
 RSP 
CR2: 003a
---[ end trace 0873e743fc645a8d ]---

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

GPF in shm_lock ipc

2015-10-12 Thread Dmitry Vyukov 
Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
0x3000ul, 0x3ul, 0x207f9000ul);
long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
0x3000ul, 0x0ul, 0x7ul, 0x0ul);
return 0;
}

On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 81bcb43c 88081bf0bd70 812fe8d6 
 88081bf0bda8 81051ff1 ffea 88081b896ca8
 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x44/0x5e lib/dump_stack.c:50
 [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
 [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
 [< inline >] shm_lock ipc/shm.c:162
 [] shm_open+0x74/0x80 ipc/shm.c:196
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
---[ end trace 0873e743fc645a8c ]---
BUG: unable to handle kernel NULL pointer dereference at 003a
IP: [] shm_open+0x35/0x80 ipc/shm.c:197
PGD 81a08b067 PUD 81a01b067 PMD 0
Oops: 0002 [#1] SMP
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Tainted: GW   4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: 880819b26800 ti: 88081bf08000 task.ti: 88081bf08000
RIP: 0010:[]  [] shm_open+0x35/0x80
RSP: 0018:88081bf0bdc8  EFLAGS: 00010296
RAX: 560d1d13 RBX: ffea RCX: 8151e710
RDX: 0001 RSI:  RDI: 0009
RBP: 88081bf0bdd0 R08: 000a R09: 0001
R10:  R11: 01bc R12: 88081b896ca8
R13: 880819b81620 R14: 8800bbaa6d00 R15: 880819b81600
FS:  7fbfd966f700() GS:88083fd0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 003a CR3: 000819a97000 CR4: 06e0
Stack:
 880819b81200 88081bf0bdf8 81295cbe 19b81620
 880819b81628 207f9000 88081bf0be80 81142d14
 8800bbaa6d00 0007  0007
Call Trace:
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
Code: 00 00 48 8b 80 d0 00 00 00 48 8b 50 08 8b 30 48 8d ba c8 00 00
00 e8 6b b1 ff ff 48 3d 00 f0 ff ff 48 89 c3 77 33 e8 5b 6b e1 ff <48>
89 43 50 65 48 8b 04 25 c0 ad 00 00 48 8b 80 40 04 00 00 48
RIP  [] shm_open+0x35/0x80 ipc/shm.c:197
 RSP 
CR2: 003a
---[ end trace 0873e743fc645a8d ]---

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

Re: GPF in shm_lock ipc

2015-10-12 Thread Dmitry Vyukov 
On Mon, Oct 12, 2015 at 1:41 PM, Vlastimil Babka  wrote:
> On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:
>>
>> Hello,
>>
>> The following program crashes kernel:
>>
>> // autogenerated by syzkaller (http://github.com/google/syzkaller)
>> #include 
>> #include 
>> #include 
>>
>> int main()
>> {
>>  long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
>>  long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
>>  long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
>> 0x3000ul, 0x3ul, 0x207f9000ul);
>>  long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
>>  long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
>> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
>>  return 0;
>> }
>>
>> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
>> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
>>
>> [ cut here ]
>> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
>> Modules linked in:
>> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>> 01/01/2011
>>   81bcb43c 88081bf0bd70 812fe8d6 
>>   88081bf0bda8 81051ff1 ffea 88081b896ca8
>>   880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
>> Call Trace:
>>   [< inline >] __dump_stack lib/dump_stack.c:15
>>   [] dump_stack+0x44/0x5e lib/dump_stack.c:50
>>   [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>>   [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>>   [< inline >] shm_lock ipc/shm.c:162
>>   [] shm_open+0x74/0x80 ipc/shm.c:196
>>   [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>>   [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>>   [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>>   [< inline >] do_mmap_pgoff include/linux/mm.h:1930
>>   [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
>
>
> Hmm what kind of stack unwinder catches inlines? Some external patch, based
> on debuginfo?

We use the following script to symbolize kernel stack traces. It adds
file:line info and inlined frames.
https://github.com/google/sanitizers/blob/master/address-sanitizer/tools/kasan_symbolize.py
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2015-10-12 Thread Kirill A. Shutemov 
On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> Hello,
> 
> The following program crashes kernel:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include 
> #include 
> #include 
> 
> int main()
> {
> long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
> long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
> long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
> 0x3000ul, 0x3ul, 0x207f9000ul);
> long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
> long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
> return 0;
> }

Here's slightly simplified and more human readable reproducer:

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

return 0;
}

> 
> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> 
> [ cut here ]
> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> Modules linked in:
> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  81bcb43c 88081bf0bd70 812fe8d6 
>  88081bf0bda8 81051ff1 ffea 88081b896ca8
>  880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
> Call Trace:
>  [< inline >] __dump_stack lib/dump_stack.c:15
>  [] dump_stack+0x44/0x5e lib/dump_stack.c:50
>  [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>  [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>  [< inline >] shm_lock ipc/shm.c:162
>  [] shm_open+0x74/0x80 ipc/shm.c:196
>  [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>  [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>  [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>  [< inline >] do_mmap_pgoff include/linux/mm.h:1930
>  [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
>  [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
>  [] entry_SYSCALL_64_fastpath+0x12/0x6a
> arch/x86/entry/entry_64.S:185
> ---[ end trace 0873e743fc645a8c ]---

Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
VMA using existing one.

I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
normal from mm POV (no special flags, etc.) and it meats remap_file_pages
criteria (shared mapping). Every fix I can think of on mm side is ugly.

Probably better to teach shm_mmap() to fall off gracefully in case of
non-existing shmid? I'm not familiar with IPC code.
Could anyone look into it?

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

Re: GPF in shm_lock ipc

2015-10-12 Thread Vlastimil Babka 

On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:

Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
 long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
 long r1 = syscall(SYS_shmat, r0, 0x2000ul, 0x0ul);
 long r2 = syscall(SYS_mremap, 0x2000ul, 0x1000ul,
0x3000ul, 0x3ul, 0x207f9000ul);
 long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
 long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
0x3000ul, 0x0ul, 0x7ul, 0x0ul);
 return 0;
}

On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  81bcb43c 88081bf0bd70 812fe8d6 
  88081bf0bda8 81051ff1 ffea 88081b896ca8
  880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
  [< inline >] __dump_stack lib/dump_stack.c:15
  [] dump_stack+0x44/0x5e lib/dump_stack.c:50
  [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
  [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
  [< inline >] shm_lock ipc/shm.c:162
  [] shm_open+0x74/0x80 ipc/shm.c:196
  [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
  [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
  [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
  [< inline >] do_mmap_pgoff include/linux/mm.h:1930
  [< inline >] SYSC_remap_file_pages mm/mmap.c:2694


Hmm what kind of stack unwinder catches inlines? Some external patch, 
based on debuginfo?


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

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:


On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
Here's slightly simplified and more human readable reproducer:

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define PAGE_SIZE 4096

int main()
{
int id;
void *p;

id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

   return 0;
}


Thanks!



On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

[ cut here ]
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 81bcb43c 88081bf0bd70 812fe8d6 
 88081bf0bda8 81051ff1 ffea 88081b896ca8
 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x44/0x5e lib/dump_stack.c:50
 [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
 [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
 [< inline >] shm_lock ipc/shm.c:162
 [] shm_open+0x74/0x80 ipc/shm.c:196
 [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [< inline >] do_mmap_pgoff include/linux/mm.h:1930
 [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
 [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
---[ end trace 0873e743fc645a8c ]---


Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
VMA using existing one.


Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
This is common to all things ipc, not only to shm. And while Linux nowadays
does enforce that nothing touch a segment marked for deletion[1], we have
contradictory scenarios where the resource is only freed once the last attached
process exits. 


[1] https://lkml.org/lkml/2015/10/12/483

So this warning used to in fact be a full BUG_ON, but ultimately the ipc
subsystem acknowledges that this situation is possible but fully blames the
user responsible, and therefore we only warn about bogus usage.


I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
normal from mm POV (no special flags, etc.) and it meats remap_file_pages
criteria (shared mapping). Every fix I can think of on mm side is ugly.

Probably better to teach shm_mmap() to fall off gracefully in case of
non-existing shmid? I'm not familiar with IPC code.
Could anyone look into it?


Yeah, this was my approach as well. Very little tested other than it solves
the above warning. Basically we don't want to be doing mmap if the segment
was deleted, thus return a corresponding error instead of triggering the
same error later on after mmaping, via shm_open(). I still need to think
a bit more about this, but seems legit if we don't hurt userspace while
at it (at least the idea, not considering any overhead in doing the idr
lookup). Thoughts?

Thanks,
Davidlohr

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 = _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();
+
ret = sfd->file->f_op->mmap(sfd->file, vma);
if (ret != 0)
return ret;
@@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct 
vm_area_struct *vma)
shm_open(vma);
 
 	return ret;

+err:
+   rcu_read_unlock();
+   return ret;
 }
 
 static int shm_release(struct inode *ino, struct file *file)







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

Re: GPF in shm_lock ipc

2015-10-12 Thread Kirill A. Shutemov 
On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
> On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
> 
> >On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> >Here's slightly simplified and more human readable reproducer:
> >
> >#define _GNU_SOURCE
> >#include 
> >#include 
> >#include 
> >#include 
> >
> >#define PAGE_SIZE 4096
> >
> >int main()
> >{
> > int id;
> > void *p;
> >
> > id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> > p = shmat(id, NULL, 0);
> > shmctl(id, IPC_RMID, NULL);
> > remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
> >
> >   return 0;
> >}
> 
> Thanks!
> 
> >>
> >>On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> >>(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> >>
> >>[ cut here ]
> >>WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> >>Modules linked in:
> >>CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> >>Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> 81bcb43c 88081bf0bd70 812fe8d6 
> >> 88081bf0bda8 81051ff1 ffea 88081b896ca8
> >> 880819b81620 8800bbaa6d00 880819b81600 88081bf0bdb8
> >>Call Trace:
> >> [< inline >] __dump_stack lib/dump_stack.c:15
> >> [] dump_stack+0x44/0x5e lib/dump_stack.c:50
> >> [] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
> >> [] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
> >> [< inline >] shm_lock ipc/shm.c:162
> >> [] shm_open+0x74/0x80 ipc/shm.c:196
> >> [] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
> >> [] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
> >> [] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
> >> [< inline >] do_mmap_pgoff include/linux/mm.h:1930
> >> [< inline >] SYSC_remap_file_pages mm/mmap.c:2694
> >> [] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
> >> [] entry_SYSCALL_64_fastpath+0x12/0x6a
> >>arch/x86/entry/entry_64.S:185
> >>---[ end trace 0873e743fc645a8c ]---
> >
> >Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
> >be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
> >VMA using existing one.
> 
> Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
> This is common to all things ipc, not only to shm. And while Linux nowadays
> does enforce that nothing touch a segment marked for deletion[1], we have
> contradictory scenarios where the resource is only freed once the last 
> attached
> process exits.
> 
> [1] https://lkml.org/lkml/2015/10/12/483
> 
> So this warning used to in fact be a full BUG_ON, but ultimately the ipc
> subsystem acknowledges that this situation is possible but fully blames the
> user responsible, and therefore we only warn about bogus usage.
> 
> >I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
> >normal from mm POV (no special flags, etc.) and it meats remap_file_pages
> >criteria (shared mapping). Every fix I can think of on mm side is ugly.
> >
> >Probably better to teach shm_mmap() to fall off gracefully in case of
> >non-existing shmid? I'm not familiar with IPC code.
> >Could anyone look into it?
> 
> Yeah, this was my approach as well. Very little tested other than it solves
> the above warning. Basically we don't want to be doing mmap if the segment
> was deleted, thus return a corresponding error instead of triggering the
> same error later on after mmaping, via shm_open(). I still need to think
> a bit more about this, but seems legit if we don't hurt userspace while
> at it (at least the idea, not considering any overhead in doing the idr
> lookup). Thoughts?
> 
> Thanks,
> Davidlohr
> 
> 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 = _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?
Shouldn't we bump shm_nattch here? Or some other refcount?


>   ret = sfd->file->f_op->mmap(sfd->file, vma);
>   if (ret != 0)
>   return ret;
> @@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct 
> vm_area_struct *vma)
>   shm_open(vma);
>   return ret;
> +err:
> + rcu_read_unlock();
> + return

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

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 = _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]


Shouldn't we bump shm_nattch here? Or some other refcount?


At least not shm_nattach, as that would acknowledge a new attachment after
a valid IPC_RMID. But the problem is also with how we check for marked for
deletion segments -- ipc_valid_object() checking the deleted flag. As such,
we always rely on explicitly checking against the deleted flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in shm_lock ipc

2015-10-12 Thread Davidlohr Bueso 

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 = _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 
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 
---
 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(_ids(ns), id);
 
 	/*

-* We raced in the idr lookup or with shm_destroy().  Either way, the
-* ID is busted.
+*