[V3,1/5] RDMA/core: Transport-independent access flags

Commit Message

Jason Gunthorpe July 10, 2015, 7:54 p.m. UTC

On Fri, Jul 10, 2015 at 02:42:45PM -0400, Tom Talpey wrote:

> >>For the proposed iSER patch the problem is very acute, iser makes a
> >>single PD and phys MR at boot time for each device. This means there
> >>is a single machine wide unchanging rkey that allows remote physical
> >>memory access. An attacker only has to repeatedly open QPs to iSER and
> >>guess rkey values until they find it. Add in likely non-crypto
> >>randomness for rkeys, and I bet it isn't even that hard to do.
> 
> The rkeys have a PD, wich cannot be forged, so it's not a matter of
> attacking, but it is most definitely a system integrity risk, as I
> mentioned earlier, a simple arithmetic offset mistake can overwrite
> anything.

Can you explain this conclusion?

As far as I can see the flow is:

pd = ib_alloc_pd();
mr = ib_get_dma_mr(pd,IB_ACCESS_REMOTE_WRITE);
qp = ib_create_qp(pd);

So at that instant, the far side of 'qp' can issue a RDMA WRITE op
with an rkey of mr->rkey. AFAIK that is what these APIs are *defined*
to do.

I don't need to forge a PD because I have control over the far
side of a QP already attached to PD. All I need to know is the rkey
number.

So the attack is, connect to iSER, guess RKEY. If wrong, reconnect,
try again. The PD doesn't stop it. The rkey doesn't change on every
connect. Eventually you find it, and then you have access to all of
physical memory.

That is a security issue.

> > From there, we can start to look at the bigger picture of cleanup up the
> >default trust domain in the kernel and user space both (and soliciting
> >feedback on that...I have a suspicion that some users will not like us
> >tightening up security as it might interfere with their usage in their
> >sequestered clusters).
> 
> All excellent points. It's not worse, and it adds important transport
> support.

If the iWarp&iSER community is happy with this security problem then
sure, go ahead with the iser patch.

> However, it's an extremely bad idea to codify writable privileged rmr's
> in the API as best practice. So under no circumstance should that become
> the long term plan.

Agreed. We should drop ib_get_dma_mr flags wrapper, and I propose
this, so we don't have to relearn this lesson again.


Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jason Gunthorpe July 10, 2015, 8:48 p.m. UTC | #1

On Fri, Jul 10, 2015 at 01:54:20PM -0600, Jason Gunthorpe wrote:
> On Fri, Jul 10, 2015 at 02:42:45PM -0400, Tom Talpey wrote:
> 
> > >>For the proposed iSER patch the problem is very acute, iser makes a
> > >>single PD and phys MR at boot time for each device. This means there
> > >>is a single machine wide unchanging rkey that allows remote physical
> > >>memory access. An attacker only has to repeatedly open QPs to iSER and
> > >>guess rkey values until they find it. Add in likely non-crypto
> > >>randomness for rkeys, and I bet it isn't even that hard to do.
> > 
> > The rkeys have a PD, wich cannot be forged, so it's not a matter of
> > attacking, but it is most definitely a system integrity risk, as I
> > mentioned earlier, a simple arithmetic offset mistake can overwrite
> > anything.
> 
> Can you explain this conclusion?

Okay, so I see, iser is client only, it doesn't create a listening QP,
so you have to trick it into connecting to a malicious server, and
that is just a trust issue as Doug points out. Presumably this patch
doesn't impact isert?

But what about NFS? It looks to me like all of the ib_get_dma_mr
calls in NFS have the possibility of having IB_ACCESS_REMOTE_WRITE
set, but only on older adaptors?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Ledford July 10, 2015, 10:33 p.m. UTC | #2

On 07/10/2015 03:54 PM, Jason Gunthorpe wrote:
> On Fri, Jul 10, 2015 at 02:42:45PM -0400, Tom Talpey wrote:
> 
>>>> For the proposed iSER patch the problem is very acute, iser makes a
>>>> single PD and phys MR at boot time for each device. This means there
>>>> is a single machine wide unchanging rkey that allows remote physical
>>>> memory access. An attacker only has to repeatedly open QPs to iSER and
>>>> guess rkey values until they find it. Add in likely non-crypto
>>>> randomness for rkeys, and I bet it isn't even that hard to do.
>>
>> The rkeys have a PD, wich cannot be forged, so it's not a matter of
>> attacking, but it is most definitely a system integrity risk, as I
>> mentioned earlier, a simple arithmetic offset mistake can overwrite
>> anything.
> 
> Can you explain this conclusion?

I think Tom's comment was referring to the fact that if you have a
trusted client, then a third party attacker can't attack your rkey
because they wouldn't have a QP in your PD and so the rkey would be
invalid for them.  Your arguments have been centered around a malicious
client, his presumed a trusted client and malicious third party.

Christoph Hellwig July 11, 2015, 10:17 a.m. UTC | #3

On Fri, Jul 10, 2015 at 01:54:20PM -0600, Jason Gunthorpe wrote:
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index bac3fb406a74..6ed7e0f6c162 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1126,6 +1126,12 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
>         struct ib_mr *mr;
>         int err;
>  
> +       /* Granting remote access to the physical MR is a security hole, don't
> +          do it. */
> +       WARN_ON_ONCE(mr_access_flags &
> +               (IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ |
> +                IB_ACCESS_REMOTE_ATOMIC));
> +

How about providing a system-wide IB_ACCESS_LOCAL_READ |
IB_ACCESS_LOCAL_WRITE MR that all drivers can use and get rid of
ib_get_dma_mr in the long run?  That would help to nicely simplify
drivers?

Currently various drivers are using ib_get_dma_mr with remote flags
unfortunately, e.g. the SRP initiator driver uses it to optimize away
memory registrtions for single SGL entry requests.  That looks fixable
realtively easily, but I don't understand the other consumers as good.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe July 13, 2015, 4:57 p.m. UTC | #4

On Sat, Jul 11, 2015 at 03:17:36AM -0700, 'Christoph Hellwig' wrote:
> On Fri, Jul 10, 2015 at 01:54:20PM -0600, Jason Gunthorpe wrote:
> > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> > index bac3fb406a74..6ed7e0f6c162 100644
> > +++ b/drivers/infiniband/core/verbs.c
> > @@ -1126,6 +1126,12 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
> >         struct ib_mr *mr;
> >         int err;
> >  
> > +       /* Granting remote access to the physical MR is a security hole, don't
> > +          do it. */
> > +       WARN_ON_ONCE(mr_access_flags &
> > +               (IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ |
> > +                IB_ACCESS_REMOTE_ATOMIC));
> > +
> 
> How about providing a system-wide IB_ACCESS_LOCAL_READ |
> IB_ACCESS_LOCAL_WRITE MR that all drivers can use and get rid of
> ib_get_dma_mr in the long run?  That would help to nicely simplify
> drivers?

That is more or less what I was talking about when I suggested
removing the lkey from the API ULPs use.

Ultimately ib_get_dma_mr is just one call, and switching to
(eg) pd->local_memory_lkey is tidier, but not much simpler..

> Currently various drivers are using ib_get_dma_mr with remote flags
> unfortunately, e.g. the SRP initiator driver uses it to optimize away
> memory registrtions for single SGL entry requests.

Unconditionally? Ugh. Maybe we do need the warn_on :(

> That looks fixable realtively easily, but I don't understand the
> other consumers as good.

The ones I looked at used it as a fallback if FMR or FRMR are not
available.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig July 14, 2015, 7:25 a.m. UTC | #5

On Mon, Jul 13, 2015 at 10:57:48AM -0600, Jason Gunthorpe wrote:
> > Currently various drivers are using ib_get_dma_mr with remote flags
> > unfortunately, e.g. the SRP initiator driver uses it to optimize away
> > memory registrtions for single SGL entry requests.
> 
> Unconditionally? Ugh. Maybe we do need the warn_on :(

There is a "register_always" flag to always use real MRs, but it's
off by default:

	if (count == 1 && !register_always) {
		/*
		 * The midlayer only generated a single gather/scatter
		 * entry, or DMA mapping coalesced everything to a
		 * single entry. So a direct descriptor along with
		 * the DMA MR suffices.
		 */
		struct srp_direct_buf *buf = (void *) cmd->add_data;

		buf->va  = cpu_to_be64(ib_sg_dma_address(ibdev, scat));
                buf->key = cpu_to_be32(target->rkey);
		buf->len = cpu_to_be32(ib_sg_dma_len(ibdev, scat));

		req->nmdesc = 0;
		goto map_complete;
	}
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sagi Grimberg July 14, 2015, 9:05 a.m. UTC | #6

On 7/14/2015 10:25 AM, 'Christoph Hellwig' wrote:
> On Mon, Jul 13, 2015 at 10:57:48AM -0600, Jason Gunthorpe wrote:
>>> Currently various drivers are using ib_get_dma_mr with remote flags
>>> unfortunately, e.g. the SRP initiator driver uses it to optimize away
>>> memory registrtions for single SGL entry requests.
>>
>> Unconditionally? Ugh. Maybe we do need the warn_on :(
>
> There is a "register_always" flag to always use real MRs, but it's
> off by default:
>
> 	if (count == 1 && !register_always) {
> 		/*
> 		 * The midlayer only generated a single gather/scatter
> 		 * entry, or DMA mapping coalesced everything to a
> 		 * single entry. So a direct descriptor along with
> 		 * the DMA MR suffices.
> 		 */
> 		struct srp_direct_buf *buf = (void *) cmd->add_data;
>
> 		buf->va  = cpu_to_be64(ib_sg_dma_address(ibdev, scat));
>                  buf->key = cpu_to_be32(target->rkey);
> 		buf->len = cpu_to_be32(ib_sg_dma_len(ibdev, scat));
>
> 		req->nmdesc = 0;
> 		goto map_complete;
> 	}
>

iser has it too. I have a similar patch with a flag for iser (its
behind a bulk of patches that are still pending though).
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig July 14, 2015, 3:35 p.m. UTC | #7

On Tue, Jul 14, 2015 at 12:05:53PM +0300, Sagi Grimberg wrote:
> iser has it too. I have a similar patch with a flag for iser (its
> behind a bulk of patches that are still pending though).

So instead of this flag can we revisit the need for it?  Given how
inherently isecture it is maybe a "alloc_insecure" option that turns
on the global REMOTE_WRITE MRs might be a better idea, both for these
hacks in iSER and SRP and the current iSER support without FRs.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe July 14, 2015, 5:26 p.m. UTC | #8

On Tue, Jul 14, 2015 at 12:05:53PM +0300, Sagi Grimberg wrote:

> iser has it too. I have a similar patch with a flag for iser (its
> behind a bulk of patches that are still pending though).

Do we all agree and understand that stuff like this in

drivers/infiniband/ulp/iser/iser_verbs.c

        device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
                                   IB_ACCESS_REMOTE_WRITE |
                                   IB_ACCESS_REMOTE_READ);

Represents a significant security risk to the machine, and must be
off be default?

Can you take care of fixing this for iser?

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sagi Grimberg July 15, 2015, 7:10 a.m. UTC | #9

On 7/14/2015 8:26 PM, Jason Gunthorpe wrote:
> On Tue, Jul 14, 2015 at 12:05:53PM +0300, Sagi Grimberg wrote:
>
>> iser has it too. I have a similar patch with a flag for iser (its
>> behind a bulk of patches that are still pending though).
>
> Do we all agree and understand that stuff like this in
>
> drivers/infiniband/ulp/iser/iser_verbs.c
>
>          device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
>                                     IB_ACCESS_REMOTE_WRITE |
>                                     IB_ACCESS_REMOTE_READ);
>
> Represents a significant security risk to the machine, and must be
> off be default?
>
> Can you take care of fixing this for iser?

I will. It is part of a patchset I have to support remote
invalidate in iser and isert.

Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index bac3fb406a74..6ed7e0f6c162 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1126,6 +1126,12 @@  struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
        struct ib_mr *mr;
        int err;
 
+       /* Granting remote access to the physical MR is a security hole, don't
+          do it. */
+       WARN_ON_ONCE(mr_access_flags &
+               (IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ |
+                IB_ACCESS_REMOTE_ATOMIC));
+
        err = ib_check_mr_access(mr_access_flags);
        if (err)
                return ERR_PTR(err);