[PATCH rdma-next 0/6] BAR mappings fixes in RDMA

[PATCH rdma-next 0/6] BAR mappings fixes in RDMA

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

>From Jason,

Upon review it turns out there are some long standing problems in BAR
mapping area:
 * BAR pages are being allowed to be executable.
 * BAR pages intended for read-only can be switched to writable via mprotect.
 * Missing use of rdma_user_mmap_io for the mlx5 clock BAR page.
 * Disassociate causes SIGBUS when touching the pages.
 * CPU pages are being mapped through to the process via remap_pfn_range
   instead of the more appropriate vm_insert_page, causing weird behaviors
   during disassociation.

This series adds the missing VM_* flag manipulation, adds faulting a zero
page for disassociation and revises the CPU page mappings to use vm_insert_page.

Thanks

Jason Gunthorpe (6):
  RDMA/mlx5: Do not allow the user to write to the clock page
  RDMA/mlx5: Use rdma_user_map_io for mapping BAR pages
  RDMA/ucontext: Do not allow BAR mappings to be executable
  RDMA/ucontext: Fix regression with disassociate
  RDMA/mlx5: Use get_zeroed_page() for clock_info
  RDMA: Remove rdma_user_mmap_page

 drivers/infiniband/core/uverbs.h              |   1 +
 drivers/infiniband/core/uverbs_main.c         | 115 ++++++++++--------
 drivers/infiniband/hw/mlx5/main.c             |  21 ++--
 .../ethernet/mellanox/mlx5/core/lib/clock.c   |  30 ++---
 include/linux/mlx5/driver.h                   |   1 -
 include/rdma/ib_verbs.h                       |   9 --
 6 files changed, 85 insertions(+), 92 deletions(-)

[PATCH rdma-next 0/6] BAR mappings fixes in RDMA

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

From Jason,

Upon review it turns out there are some long standing problems in BAR
mapping area:
 * BAR pages are being allowed to be executable.
 * BAR pages intended for read-only can be switched to writable via mprotect.
 * Missing use of rdma_user_mmap_io for the mlx5 clock BAR page.
 * Disassociate causes SIGBUS when touching the pages.
 * CPU pages are being mapped through to the process via remap_pfn_range
   instead of the more appropriate vm_insert_page, causing weird behaviors
   during disassociation.

This series adds the missing VM_* flag manipulation, adds faulting a zero
page for disassociation and revises the CPU page mappings to use vm_insert_page.

Thanks

Jason Gunthorpe (6):
  RDMA/mlx5: Do not allow the user to write to the clock page
  RDMA/mlx5: Use rdma_user_map_io for mapping BAR pages
  RDMA/ucontext: Do not allow BAR mappings to be executable
  RDMA/ucontext: Fix regression with disassociate
  RDMA/mlx5: Use get_zeroed_page() for clock_info
  RDMA: Remove rdma_user_mmap_page

 drivers/infiniband/core/uverbs.h              |   1 +
 drivers/infiniband/core/uverbs_main.c         | 115 ++++++++++--------
 drivers/infiniband/hw/mlx5/main.c             |  21 ++--
 .../ethernet/mellanox/mlx5/core/lib/clock.c   |  30 ++---
 include/linux/mlx5/driver.h                   |   1 -
 include/rdma/ib_verbs.h                       |   9 --
 6 files changed, 85 insertions(+), 92 deletions(-)

--
2.20.1

[PATCH rdma-next 1/6] RDMA/mlx5: Do not allow the user to write to the clock page

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

The intent of this VMA was to be read-only from user space, but the
VM_MAYWRITE masking was missed, so mprotect could make it writable.

Cc: stable@vger.kernel.org
Fixes: 5c99eaecb1fc ("IB/mlx5: Mmap the HCA's clock info to user-space")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 15676a149680..73b7f9636f82 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2072,6 +2072,7 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev,
 
 	if (vma->vm_flags & VM_WRITE)
 		return -EPERM;
+	vma->vm_flags &= ~VM_MAYWRITE;
 
 	if (!dev->mdev->clock_info_page)
 		return -EOPNOTSUPP;
@@ -2237,6 +2238,7 @@ static int mlx5_ib_mmap(struct ib_ucontext *ibcontext, struct vm_area_struct *vm
 
 		if (vma->vm_flags & VM_WRITE)
 			return -EPERM;
+		vma->vm_flags &= ~VM_MAYWRITE;
 
 		/* Don't expose to user-space information it shouldn't have */
 		if (PAGE_SIZE > 4096)
-- 
2.20.1

[PATCH rdma-next 2/6] RDMA/mlx5: Use rdma_user_map_io for mapping BAR pages

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

Since mlx5 supports device disassociate it must use this API for all
BAR page mmaps, otherwise the pages can remain mapped after the device
is unplugged causing a system crash.

Cc: stable@vger.kernel.org
Fixes: 5f9794dc94f5 ("RDMA/ucontext: Add a core API for mmaping driver IO memory")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/main.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 73b7f9636f82..77b018c0923f 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2244,14 +2244,12 @@ static int mlx5_ib_mmap(struct ib_ucontext *ibcontext, struct vm_area_struct *vm
 		if (PAGE_SIZE > 4096)
 			return -EOPNOTSUPP;
 
-		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 		pfn = (dev->mdev->iseg_base +
 		       offsetof(struct mlx5_init_seg, internal_timer_h)) >>
 			PAGE_SHIFT;
-		if (io_remap_pfn_range(vma, vma->vm_start, pfn,
-				       PAGE_SIZE, vma->vm_page_prot))
-			return -EAGAIN;
-		break;
+		return rdma_user_mmap_io(&context->ibucontext, vma, pfn,
+					 PAGE_SIZE,
+					 pgprot_noncached(vma->vm_page_prot));
 	case MLX5_IB_MMAP_CLOCK_INFO:
 		return mlx5_ib_mmap_clock_info_page(dev, vma, context);
 
-- 
2.20.1

[PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

These are not code pages and should never be executed.

Cc: stable@vger.kernel.org
Fixes: 5f9794dc94f5 ("RDMA/ucontext: Add a core API for mmaping driver IO memory")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index fef4519d1241..3ef6474cd201 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -889,6 +889,10 @@ static struct rdma_umap_priv *rdma_user_mmap_pre(struct ib_ucontext *ucontext,
 	struct ib_uverbs_file *ufile = ucontext->ufile;
 	struct rdma_umap_priv *priv;
 
+	if (vma->vm_flags & VM_EXEC)
+		return ERR_PTR(-EINVAL);
+	vma->vm_flags &= ~VM_MAYEXEC;
+
 	if (vma->vm_end - vma->vm_start != size)
 		return ERR_PTR(-EINVAL);
 
-- 
2.20.1

RE: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Ruhl, Michael J]

>-----Original Message-----
>From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>owner@vger.kernel.org] On Behalf Of Leon Romanovsky
>Sent: Tuesday, April 16, 2019 4:07 AM
>To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
><jgg@mellanox.com>
>Cc: Leon Romanovsky <leonro@mellanox.com>; RDMA mailing list <linux-
>rdma@vger.kernel.org>; Andrea Arcangeli <aarcange@redhat.com>; Feras
>Daoud <ferasda@mellanox.com>; Haggai Eran <haggaie@mellanox.com>;
>Jason Gunthorpe <jgg@ziepe.ca>; Saeed Mahameed
><saeedm@mellanox.com>; linux-netdev <netdev@vger.kernel.org>
>Subject: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings
>to be executable
>
>From: Jason Gunthorpe <jgg@mellanox.com>
>
>These are not code pages and should never be executed.
>
>Cc: stable@vger.kernel.org
>Fixes: 5f9794dc94f5 ("RDMA/ucontext: Add a core API for mmaping driver IO
>memory")
>Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
>Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>---
> drivers/infiniband/core/uverbs_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/infiniband/core/uverbs_main.c
>b/drivers/infiniband/core/uverbs_main.c
>index fef4519d1241..3ef6474cd201 100644
>--- a/drivers/infiniband/core/uverbs_main.c
>+++ b/drivers/infiniband/core/uverbs_main.c
>@@ -889,6 +889,10 @@ static struct rdma_umap_priv
>*rdma_user_mmap_pre(struct ib_ucontext *ucontext,
> 	struct ib_uverbs_file *ufile = ucontext->ufile;
> 	struct rdma_umap_priv *priv;
>
>+	if (vma->vm_flags & VM_EXEC)
>+		return ERR_PTR(-EINVAL);
>+	vma->vm_flags &= ~VM_MAYEXEC;
>+

A change like this was made in HFI with:

commit 12220267645cb7d1f3f699218e0098629e932e1f
IB/hfi: Protect against writable mmap

This caused user applications that use the stack for execution to fail.
The VM_EXEC flag is passed down during mmaps.

We had to remove this patch with:

commit 7709b0dc265f28695487712c45f02bbd1f98415d
IB/hfi1: Remove overly conservative VM_EXEC flag check

to resolve this issue.

I am not sure if this is an equivalent issue, but the code path
appears very similar.

Testing with applications that set the EXECSTACK bit will show if this
is an issue.

Mike

> 	if (vma->vm_end - vma->vm_start != size)
> 		return ERR_PTR(-EINVAL);
>
>--
>2.20.1

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Jason Gunthorpe]

On Wed, Apr 17, 2019 at 07:05:37PM +0000, Ruhl, Michael J wrote:

> >diff --git a/drivers/infiniband/core/uverbs_main.c
> >b/drivers/infiniband/core/uverbs_main.c
> >index fef4519d1241..3ef6474cd201 100644
> >+++ b/drivers/infiniband/core/uverbs_main.c
> >@@ -889,6 +889,10 @@ static struct rdma_umap_priv
> >*rdma_user_mmap_pre(struct ib_ucontext *ucontext,
> > 	struct ib_uverbs_file *ufile = ucontext->ufile;
> > 	struct rdma_umap_priv *priv;
> >
> >+	if (vma->vm_flags & VM_EXEC)
> >+		return ERR_PTR(-EINVAL);
> >+	vma->vm_flags &= ~VM_MAYEXEC;
> >+
> 
> A change like this was made in HFI with:
> 
> commit 12220267645cb7d1f3f699218e0098629e932e1f
> IB/hfi: Protect against writable mmap
> 
> This caused user applications that use the stack for execution to fail.
> The VM_EXEC flag is passed down during mmaps.
> 
> We had to remove this patch with:
> 
> commit 7709b0dc265f28695487712c45f02bbd1f98415d
> IB/hfi1: Remove overly conservative VM_EXEC flag check
> 
> to resolve this issue.
> 
> I am not sure if this is an equivalent issue, but the code path
> appears very similar.

It does seem problematic here too

Kees: You have worked in this W^X area in other parts of the kernel,
what should drivers do here?

The situation is we have a driver providing mmap against BAR memory
that is absolutely not intended for execution, so we would prefer to
block VM_EXEC in the driver's mmap fops callback

However READ_IMPLIES_EXEC forces VM_EXEC on for everything with no way
to opt out..

Jason

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Kees Cook]

On Thu, Apr 18, 2019 at 12:58 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Wed, Apr 17, 2019 at 07:05:37PM +0000, Ruhl, Michael J wrote:
>
> > >diff --git a/drivers/infiniband/core/uverbs_main.c
> > >b/drivers/infiniband/core/uverbs_main.c
> > >index fef4519d1241..3ef6474cd201 100644
> > >+++ b/drivers/infiniband/core/uverbs_main.c
> > >@@ -889,6 +889,10 @@ static struct rdma_umap_priv
> > >*rdma_user_mmap_pre(struct ib_ucontext *ucontext,
> > >     struct ib_uverbs_file *ufile = ucontext->ufile;
> > >     struct rdma_umap_priv *priv;
> > >
> > >+    if (vma->vm_flags & VM_EXEC)
> > >+            return ERR_PTR(-EINVAL);
> > >+    vma->vm_flags &= ~VM_MAYEXEC;
> > >+
> >
> > A change like this was made in HFI with:
> >
> > commit 12220267645cb7d1f3f699218e0098629e932e1f
> > IB/hfi: Protect against writable mmap
> >
> > This caused user applications that use the stack for execution to fail.
> > The VM_EXEC flag is passed down during mmaps.
> >
> > We had to remove this patch with:
> >
> > commit 7709b0dc265f28695487712c45f02bbd1f98415d
> > IB/hfi1: Remove overly conservative VM_EXEC flag check
> >
> > to resolve this issue.
> >
> > I am not sure if this is an equivalent issue, but the code path
> > appears very similar.
>
> It does seem problematic here too
>
> Kees: You have worked in this W^X area in other parts of the kernel,
> what should drivers do here?
>
> The situation is we have a driver providing mmap against BAR memory
> that is absolutely not intended for execution, so we would prefer to
> block VM_EXEC in the driver's mmap fops callback
>
> However READ_IMPLIES_EXEC forces VM_EXEC on for everything with no way
> to opt out..

Ah, my old "friend" READ_IMPLIES_EXEC!

Anything running with READ_IMPLIES_EXEC (i.e. a gnu stack marked WITH
execute) should be considered broken. Now, the trouble is that this
personality flag is carried across execve(), so if you have a launcher
that doesn't fix up the personality for children, you'll see this
spread all over your process tree. What is doing rdma mmap calls with
an executable stack? That really feels to me like the real source of
the problem.

I swear this was different handling of READ_IMPLIES_EXEC between
x86_64 and ia32, but I can't find it. (i.e. I thought the default for
64-bit was to assume NX stack even when the gnustack marking was
missing.)

Is the file for the driver coming out of /dev? Seems like that should
be mounted noexec and it would solve this too. (Though now I wonder
why /dev isn't noexec by default? /dev/pts is noexec...

Regardless, if you wanted to add a "ignore READ_IMPLIES_EXEC" flag to
struct file, maybe this bit could be populated by drivers?

-- 
Kees Cook

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Jason Gunthorpe]

On Thu, Apr 18, 2019 at 01:30:07AM -0500, Kees Cook wrote:

> Anything running with READ_IMPLIES_EXEC (i.e. a gnu stack marked WITH
> execute) should be considered broken. Now, the trouble is that this
> personality flag is carried across execve(), so if you have a launcher
> that doesn't fix up the personality for children, you'll see this
> spread all over your process tree. What is doing rdma mmap calls with
> an executable stack? That really feels to me like the real source of
> the problem.

Apparently the Fortran runtime forces the READ_IMPLIES_EXEC and
requires it for some real reason or another - Fortran and RDMA go
together in alot of cases.
 
> Is the file for the driver coming out of /dev? Seems like that should
> be mounted noexec and it would solve this too. (Though now I wonder
> why /dev isn't noexec by default? /dev/pts is noexec...

Yes - maybe?
 
> Regardless, if you wanted to add a "ignore READ_IMPLIES_EXEC" flag to
> struct file, maybe this bit could be populated by drivers?

This would solve our problem.. How about a flag in struct
file_operations?

Do you agree it is worth drivers banning VM_EXEC for these truely
non-executable pages?

Thanks,
Jason

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Kees Cook]

On Thu, Apr 18, 2019 at 2:01 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Apr 18, 2019 at 01:30:07AM -0500, Kees Cook wrote:
>
> > Anything running with READ_IMPLIES_EXEC (i.e. a gnu stack marked WITH
> > execute) should be considered broken. Now, the trouble is that this
> > personality flag is carried across execve(), so if you have a launcher
> > that doesn't fix up the personality for children, you'll see this
> > spread all over your process tree. What is doing rdma mmap calls with
> > an executable stack? That really feels to me like the real source of
> > the problem.
>
> Apparently the Fortran runtime forces the READ_IMPLIES_EXEC and
> requires it for some real reason or another - Fortran and RDMA go
> together in alot of cases.

That's pretty unfortunate for the security of the resulting proceses. :(

> > Is the file for the driver coming out of /dev? Seems like that should
> > be mounted noexec and it would solve this too. (Though now I wonder
> > why /dev isn't noexec by default? /dev/pts is noexec...
>
> Yes - maybe?

I've found why /dev isn't noexec: to support old tools that mapped
/dev/zero with VM_EXEC to get executable mappings (instead of using
MAP_ANON). Seems like maybe this could change now?

> > Regardless, if you wanted to add a "ignore READ_IMPLIES_EXEC" flag to
> > struct file, maybe this bit could be populated by drivers?
>
> This would solve our problem.. How about a flag in struct
> file_operations?

Oh! That seems like it'd be pretty clean, I think. There's no flags
field currently, which vaguely surprises me...

I wonder if we could simply make devtmpfs ignore READ_IMPLIES_EXEC
entirely, though? And I wonder if we could defang READ_IMPLIES_EXEC a
bit in general. It was _supposed_ to be for the cases where binaries
were missing exec bits and a processor was just gaining NX ability. I
know this has been discussed before... ah-ha, here it is:
http://lkml.kernel.org/r/1462963502-11636-1-git-send-email-hecmargi@upv.es

> Do you agree it is worth drivers banning VM_EXEC for these truely
> non-executable pages?

I do: I think it's reasonable defense-in-depth.

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Kees Cook]

On Thu, Apr 18, 2019 at 2:01 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Apr 18, 2019 at 01:30:07AM -0500, Kees Cook wrote:
>
> > Anything running with READ_IMPLIES_EXEC (i.e. a gnu stack marked WITH
> > execute) should be considered broken. Now, the trouble is that this
> > personality flag is carried across execve(), so if you have a launcher
> > that doesn't fix up the personality for children, you'll see this
> > spread all over your process tree. What is doing rdma mmap calls with
> > an executable stack? That really feels to me like the real source of
> > the problem.
>
> Apparently the Fortran runtime forces the READ_IMPLIES_EXEC and
> requires it for some real reason or another - Fortran and RDMA go
> together in alot of cases.

That's pretty unfortunate for the security of the resulting proceses. :(

> > Is the file for the driver coming out of /dev? Seems like that should
> > be mounted noexec and it would solve this too. (Though now I wonder
> > why /dev isn't noexec by default? /dev/pts is noexec...
>
> Yes - maybe?

I've found why /dev isn't noexec: to support old tools that mapped
/dev/zero with VM_EXEC to get executable mappings (instead of using
MAP_ANON). Seems like maybe this could change now?

> > Regardless, if you wanted to add a "ignore READ_IMPLIES_EXEC" flag to
> > struct file, maybe this bit could be populated by drivers?
>
> This would solve our problem.. How about a flag in struct
> file_operations?

Oh! That seems like it'd be pretty clean, I think. There's no flags
field currently, which vaguely surprises me...

I wonder if we could simply make devtmpfs ignore READ_IMPLIES_EXEC
entirely, though? And I wonder if we could defang READ_IMPLIES_EXEC a
bit in general. It was _supposed_ to be for the cases where binaries
were missing exec bits and a processor was just gaining NX ability. I
know this has been discussed before... ah-ha, here it is:
http://lkml.kernel.org/r/1462963502-11636-1-git-send-email-hecmargi@upv.es

> Do you agree it is worth drivers banning VM_EXEC for these truely
> non-executable pages?

I do: I think it's reasonable defense-in-depth.

--
Kees Cook

Re: [PATCH rdma-next 3/6] RDMA/ucontext: Do not allow BAR mappings to be executable

[Jason Gunthorpe]

On Thu, Apr 18, 2019 at 02:23:26AM -0500, Kees Cook wrote:
> On Thu, Apr 18, 2019 at 2:01 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Thu, Apr 18, 2019 at 01:30:07AM -0500, Kees Cook wrote:
> >
> > > Anything running with READ_IMPLIES_EXEC (i.e. a gnu stack marked WITH
> > > execute) should be considered broken. Now, the trouble is that this
> > > personality flag is carried across execve(), so if you have a launcher
> > > that doesn't fix up the personality for children, you'll see this
> > > spread all over your process tree. What is doing rdma mmap calls with
> > > an executable stack? That really feels to me like the real source of
> > > the problem.
> >
> > Apparently the Fortran runtime forces the READ_IMPLIES_EXEC and
> > requires it for some real reason or another - Fortran and RDMA go
> > together in alot of cases.
> 
> That's pretty unfortunate for the security of the resulting proceses. :(

I think it probably arises from a need for exec stacks in the
runtime... That Linux escalates that to full READ_IMPLIES_EXEC seems
quite unfortunate. Hopefully your patch will get accepted as it makes
a lot of sense.

> I wonder if we could simply make devtmpfs ignore READ_IMPLIES_EXEC
> entirely, though? And I wonder if we could defang READ_IMPLIES_EXEC a
> bit in general. It was _supposed_ to be for the cases where binaries
> were missing exec bits and a processor was just gaining NX ability. I
> know this has been discussed before... ah-ha, here it is:
> http://lkml.kernel.org/r/1462963502-11636-1-git-send-email-hecmargi@upv.es

Globally banning VM_EXEC from char device nodes also sounds very
appealing to me (particularly from a W^X sense)... There are not very
many grep hits on VM_EXEC in drivers/*, and none of the ones I looked
at seemed problematic.

Jason

[PATCH rdma-next 4/6] RDMA/ucontext: Fix regression with disassociate

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

When this code was consolidated the intention was that the VMA would
become backed by anonymous zero pages after the zap_vma_pte - however this
very subtly relied on setting the vm_ops = NULL and clearing the VM_SHARED
bits to transform the VMA into an anonymous VMA. Since the vm_ops was
removed this broke.

Now userspace gets a SIGBUS if it touches the vma after disassociation.

Instead of converting the VMA to anonymous provide a fault handler that
puts a zero'd page into the VMA when user-space touches it after
disassociation.

Cc: stable@vger.kernel.org
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Fixes: 5f9794dc94f5 ("RDMA/ucontext: Add a core API for mmaping driver IO memory")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs.h      |  1 +
 drivers/infiniband/core/uverbs_main.c | 51 +++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 0fc71ad42490..d2c29868172c 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -160,6 +160,7 @@ struct ib_uverbs_file {
 
 	struct mutex umap_lock;
 	struct list_head umaps;
+	struct page *disassociate_page;
 
 	struct idr		idr;
 	/* spinlock protects write access to idr */
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 3ef6474cd201..4a7cf5fddaee 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -208,6 +208,9 @@ void ib_uverbs_release_file(struct kref *ref)
 		kref_put(&file->async_file->ref,
 			 ib_uverbs_release_async_event_file);
 	put_device(&file->device->dev);
+
+	if (file->disassociate_page)
+		__free_pages(file->disassociate_page, 0);
 	kfree(file);
 }
 
@@ -877,9 +880,50 @@ static void rdma_umap_close(struct vm_area_struct *vma)
 	kfree(priv);
 }
 
+/*
+ * Once the zap_vma_ptes has been called touches to the VMA will come here and
+ * we return a dummy writable zero page for all the pfns.
+ */
+static vm_fault_t rdma_umap_fault(struct vm_fault *vmf)
+{
+	struct ib_uverbs_file *ufile = vmf->vma->vm_file->private_data;
+	struct rdma_umap_priv *priv = vmf->vma->vm_private_data;
+	vm_fault_t ret = 0;
+
+	if (!priv)
+		return VM_FAULT_SIGBUS;
+
+	/* Read only pages can just use the system zero page. */
+	if (!(vmf->vma->vm_flags & (VM_WRITE | VM_MAYWRITE))) {
+		vmf->page = ZERO_PAGE(vmf->vm_start);
+		get_page(vmf->page);
+		return 0;
+	}
+
+	mutex_lock(&ufile->umap_lock);
+	if (!ufile->disassociate_page)
+		ufile->disassociate_page =
+			alloc_pages(vmf->gfp_mask | __GFP_ZERO, 0);
+
+	if (ufile->disassociate_page) {
+		/*
+		 * This VMA is forced to always be shared so this doesn't have
+		 * to worry about COW.
+		 */
+		vmf->page = ufile->disassociate_page;
+		get_page(vmf->page);
+	} else {
+		ret = VM_FAULT_SIGBUS;
+	}
+	mutex_unlock(&ufile->umap_lock);
+
+	return ret;
+}
+
 static const struct vm_operations_struct rdma_umap_ops = {
 	.open = rdma_umap_open,
 	.close = rdma_umap_close,
+	.fault = rdma_umap_fault,
 };
 
 static struct rdma_umap_priv *rdma_user_mmap_pre(struct ib_ucontext *ucontext,
@@ -889,6 +933,8 @@ static struct rdma_umap_priv *rdma_user_mmap_pre(struct ib_ucontext *ucontext,
 	struct ib_uverbs_file *ufile = ucontext->ufile;
 	struct rdma_umap_priv *priv;
 
+	if (!(vma->vm_flags & VM_SHARED))
+		return ERR_PTR(-EINVAL);
 	if (vma->vm_flags & VM_EXEC)
 		return ERR_PTR(-EINVAL);
 	vma->vm_flags &= ~VM_MAYEXEC;
@@ -996,7 +1042,7 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
 		 * at a time to get the lock ordering right. Typically there
 		 * will only be one mm, so no big deal.
 		 */
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
 		mutex_lock(&ufile->umap_lock);
 		list_for_each_entry_safe (priv, next_priv, &ufile->umaps,
 					  list) {
@@ -1008,10 +1054,9 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
 
 			zap_vma_ptes(vma, vma->vm_start,
 				     vma->vm_end - vma->vm_start);
-			vma->vm_flags &= ~(VM_SHARED | VM_MAYSHARE);
 		}
 		mutex_unlock(&ufile->umap_lock);
-		up_write(&mm->mmap_sem);
+		up_read(&mm->mmap_sem);
 		mmput(mm);
 	}
 }
-- 
2.20.1

[PATCH mlx5-next 5/6] RDMA/mlx5: Use get_zeroed_page() for clock_info

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

get_zeroed_page() returns a virtual address for the page which is better
than allocating a struct page and doing a permanent kmap on it.

Cc: stable@vger.kernel.org
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/main.c             |  5 ++--
 .../ethernet/mellanox/mlx5/core/lib/clock.c   | 30 +++++++------------
 include/linux/mlx5/driver.h                   |  1 -
 3 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 77b018c0923f..9a718258b8eb 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2074,11 +2074,12 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev,
 		return -EPERM;
 	vma->vm_flags &= ~VM_MAYWRITE;
 
-	if (!dev->mdev->clock_info_page)
+	if (!dev->mdev->clock_info)
 		return -EOPNOTSUPP;
 
 	return rdma_user_mmap_page(&context->ibucontext, vma,
-				   dev->mdev->clock_info_page, PAGE_SIZE);
+				   virt_to_page(dev->mdev->clock_info),
+				   PAGE_SIZE);
 }
 
 static int uar_mmap(struct mlx5_ib_dev *dev, enum mlx5_ib_mmap_cmd cmd,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
index ca0ee9916e9e..0059b290e095 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
@@ -535,23 +535,16 @@ void mlx5_init_clock(struct mlx5_core_dev *mdev)
 	do_div(ns, NSEC_PER_SEC / HZ);
 	clock->overflow_period = ns;
 
-	mdev->clock_info_page = alloc_page(GFP_KERNEL);
-	if (mdev->clock_info_page) {
-		mdev->clock_info = kmap(mdev->clock_info_page);
-		if (!mdev->clock_info) {
-			__free_page(mdev->clock_info_page);
-			mlx5_core_warn(mdev, "failed to map clock page\n");
-		} else {
-			mdev->clock_info->sign   = 0;
-			mdev->clock_info->nsec   = clock->tc.nsec;
-			mdev->clock_info->cycles = clock->tc.cycle_last;
-			mdev->clock_info->mask   = clock->cycles.mask;
-			mdev->clock_info->mult   = clock->nominal_c_mult;
-			mdev->clock_info->shift  = clock->cycles.shift;
-			mdev->clock_info->frac   = clock->tc.frac;
-			mdev->clock_info->overflow_period =
-						clock->overflow_period;
-		}
+	mdev->clock_info =
+		(struct mlx5_ib_clock_info *)get_zeroed_page(GFP_KERNEL);
+	if (mdev->clock_info) {
+		mdev->clock_info->nsec = clock->tc.nsec;
+		mdev->clock_info->cycles = clock->tc.cycle_last;
+		mdev->clock_info->mask = clock->cycles.mask;
+		mdev->clock_info->mult = clock->nominal_c_mult;
+		mdev->clock_info->shift = clock->cycles.shift;
+		mdev->clock_info->frac = clock->tc.frac;
+		mdev->clock_info->overflow_period = clock->overflow_period;
 	}
 
 	INIT_WORK(&clock->pps_info.out_work, mlx5_pps_out);
@@ -599,8 +592,7 @@ void mlx5_cleanup_clock(struct mlx5_core_dev *mdev)
 	cancel_delayed_work_sync(&clock->overflow_work);
 
 	if (mdev->clock_info) {
-		kunmap(mdev->clock_info_page);
-		__free_page(mdev->clock_info_page);
+		free_page((unsigned long)mdev->clock_info);
 		mdev->clock_info = NULL;
 	}
 
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 6c43191c0186..de4a493de3b9 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -681,7 +681,6 @@ struct mlx5_core_dev {
 #endif
 	struct mlx5_clock        clock;
 	struct mlx5_ib_clock_info  *clock_info;
-	struct page             *clock_info_page;
 	struct mlx5_fw_tracer   *tracer;
 };
 
-- 
2.20.1

[PATCH rdma-next 6/6] RDMA: Remove rdma_user_mmap_page

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@mellanox.com>

Upon further research drivers that want this should simply call the core
function vm_insert_page(). The VMA holds a reference on the page and it
will be automatically freed when the last reference drops. No need for
disassociate to sequence the cleanup.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_main.c | 64 +++++----------------------
 drivers/infiniband/hw/mlx5/main.c     | 12 ++---
 include/rdma/ib_verbs.h               |  9 ----
 3 files changed, 18 insertions(+), 67 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 4a7cf5fddaee..d252825930f6 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -926,46 +926,35 @@ static const struct vm_operations_struct rdma_umap_ops = {
 	.fault = rdma_umap_fault,
 };
 
-static struct rdma_umap_priv *rdma_user_mmap_pre(struct ib_ucontext *ucontext,
-						 struct vm_area_struct *vma,
-						 unsigned long size)
+/*
+ * Map IO memory into a process. This is to be called by drivers as part of
+ * their mmap() functions if they wish to send something like PCI-E BAR memory
+ * to userspace.
+ */
+int rdma_user_mmap_io(struct ib_ucontext *ucontext, struct vm_area_struct *vma,
+		      unsigned long pfn, unsigned long size, pgprot_t prot)
 {
 	struct ib_uverbs_file *ufile = ucontext->ufile;
 	struct rdma_umap_priv *priv;
 
 	if (!(vma->vm_flags & VM_SHARED))
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	if (vma->vm_flags & VM_EXEC)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	vma->vm_flags &= ~VM_MAYEXEC;
 
 	if (vma->vm_end - vma->vm_start != size)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	/* Driver is using this wrong, must be called by ib_uverbs_mmap */
 	if (WARN_ON(!vma->vm_file ||
 		    vma->vm_file->private_data != ufile))
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	lockdep_assert_held(&ufile->device->disassociate_srcu);
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-		return ERR_PTR(-ENOMEM);
-	return priv;
-}
-
-/*
- * Map IO memory into a process. This is to be called by drivers as part of
- * their mmap() functions if they wish to send something like PCI-E BAR memory
- * to userspace.
- */
-int rdma_user_mmap_io(struct ib_ucontext *ucontext, struct vm_area_struct *vma,
-		      unsigned long pfn, unsigned long size, pgprot_t prot)
-{
-	struct rdma_umap_priv *priv = rdma_user_mmap_pre(ucontext, vma, size);
-
-	if (IS_ERR(priv))
-		return PTR_ERR(priv);
+		return -ENOMEM;
 
 	vma->vm_page_prot = prot;
 	if (io_remap_pfn_range(vma, vma->vm_start, pfn, size, prot)) {
@@ -978,35 +967,6 @@ int rdma_user_mmap_io(struct ib_ucontext *ucontext, struct vm_area_struct *vma,
 }
 EXPORT_SYMBOL(rdma_user_mmap_io);
 
-/*
- * The page case is here for a slightly different reason, the driver expects
- * to be able to free the page it is sharing to user space when it destroys
- * its ucontext, which means we need to zap the user space references.
- *
- * We could handle this differently by providing an API to allocate a shared
- * page and then only freeing the shared page when the last ufile is
- * destroyed.
- */
-int rdma_user_mmap_page(struct ib_ucontext *ucontext,
-			struct vm_area_struct *vma, struct page *page,
-			unsigned long size)
-{
-	struct rdma_umap_priv *priv = rdma_user_mmap_pre(ucontext, vma, size);
-
-	if (IS_ERR(priv))
-		return PTR_ERR(priv);
-
-	if (remap_pfn_range(vma, vma->vm_start, page_to_pfn(page), size,
-			    vma->vm_page_prot)) {
-		kfree(priv);
-		return -EAGAIN;
-	}
-
-	rdma_umap_priv_init(priv, vma);
-	return 0;
-}
-EXPORT_SYMBOL(rdma_user_mmap_page);
-
 void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
 {
 	struct rdma_umap_priv *priv, *next_priv;
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 9a718258b8eb..692759914d20 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2064,22 +2064,22 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev,
 					struct vm_area_struct *vma,
 					struct mlx5_ib_ucontext *context)
 {
-	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
+	if ((vma->vm_end - vma->vm_start != PAGE_SIZE) ||
+	    !(vma->vm_flags & VM_SHARED))
 		return -EINVAL;
 
 	if (get_index(vma->vm_pgoff) != MLX5_IB_CLOCK_INFO_V1)
 		return -EOPNOTSUPP;
 
-	if (vma->vm_flags & VM_WRITE)
+	if (vma->vm_flags & (VM_WRITE | VM_EXEC))
 		return -EPERM;
-	vma->vm_flags &= ~VM_MAYWRITE;
+	vma->vm_flags &= ~(VM_MAYWRITE | VM_MAYEXEC);
 
 	if (!dev->mdev->clock_info)
 		return -EOPNOTSUPP;
 
-	return rdma_user_mmap_page(&context->ibucontext, vma,
-				   virt_to_page(dev->mdev->clock_info),
-				   PAGE_SIZE);
+	return vm_insert_page(vma, vma->vm_start,
+			      virt_to_page(dev->mdev->clock_info));
 }
 
 static int uar_mmap(struct mlx5_ib_dev *dev, enum mlx5_ib_mmap_cmd cmd,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index cc311f14f41a..8014dec3bd07 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2754,9 +2754,6 @@ void ib_set_device_ops(struct ib_device *device,
 #if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
 int rdma_user_mmap_io(struct ib_ucontext *ucontext, struct vm_area_struct *vma,
 		      unsigned long pfn, unsigned long size, pgprot_t prot);
-int rdma_user_mmap_page(struct ib_ucontext *ucontext,
-			struct vm_area_struct *vma, struct page *page,
-			unsigned long size);
 #else
 static inline int rdma_user_mmap_io(struct ib_ucontext *ucontext,
 				    struct vm_area_struct *vma,
@@ -2765,12 +2762,6 @@ static inline int rdma_user_mmap_io(struct ib_ucontext *ucontext,
 {
 	return -EINVAL;
 }
-static inline int rdma_user_mmap_page(struct ib_ucontext *ucontext,
-				struct vm_area_struct *vma, struct page *page,
-				unsigned long size)
-{
-	return -EINVAL;
-}
 #endif
 
 static inline int ib_copy_from_udata(void *dest, struct ib_udata *udata, size_t len)
-- 
2.20.1

Re: [PATCH rdma-next 0/6] BAR mappings fixes in RDMA

[Jason Gunthorpe]

On Tue, Apr 16, 2019 at 02:07:24PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> >From Jason,
> 
> Upon review it turns out there are some long standing problems in BAR
> mapping area:
>  * BAR pages are being allowed to be executable.
>  * BAR pages intended for read-only can be switched to writable via mprotect.
>  * Missing use of rdma_user_mmap_io for the mlx5 clock BAR page.
>  * Disassociate causes SIGBUS when touching the pages.
>  * CPU pages are being mapped through to the process via remap_pfn_range
>    instead of the more appropriate vm_insert_page, causing weird behaviors
>    during disassociation.
> 
> This series adds the missing VM_* flag manipulation, adds faulting a zero
> page for disassociation and revises the CPU page mappings to use vm_insert_page.
> 
> Thanks
> 
> Jason Gunthorpe (6):
>   RDMA/mlx5: Do not allow the user to write to the clock page
>   RDMA/mlx5: Use rdma_user_map_io for mapping BAR pages
>   RDMA/ucontext: Fix regression with disassociate

Applied to for-rc

>   RDMA/mlx5: Use get_zeroed_page() for clock_info
>   RDMA: Remove rdma_user_mmap_page

Applied to for-next

I dropped this patch:

>   RDMA/ucontext: Do not allow BAR mappings to be executable

Jason