| Message ID | 20190416110730.32230-4-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | BAR mappings fixes in RDMA | expand |
>-----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
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
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?
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
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
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
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);