On Thu, Jul 23, 2015 at 06:47:26AM -0700, Bart Van Assche wrote:
> On 07/22/15 16:34, Jason Gunthorpe wrote:
> >The remaining users of ib_get_dma_mr are all unsafe:
> > [ ... ]
> > drivers/infiniband/ulp/srp/ib_srp.c:
> > srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
> > IB_ACCESS_LOCAL_WRITE |
> > IB_ACCESS_REMOTE_READ |
> > IB_ACCESS_REMOTE_WRITE);
>
> Hello Jason,
>
> This statement might need some clarification. Are you aware that this memory
> region is only used if the kernel module parameter register_always is zero ?
It doesn't matter, just making the above call is enough to create the
security problem, even if the rkey is not used.
I looked at the register_always stuff, it isn't obvious to me that it
interacts with this path:
static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
if (dev->use_fast_reg) {
} else {
use_mr = !!ch->fmr_pool;
[..]
if (srp_map_sg_entry(state, ch, sg, i, use_mr)) {
static int srp_map_sg_entry(struct srp_map_state *state,
if (!use_mr) {
srp_map_desc(state, dma_addr, dma_len, target->rkey);
Perhaps the above is dead code, all our IB drivers either support FMR
or FRWR, so maybe use_mr is always true?
It looks to me like register_always is similar to iSER, it is trying
to avoid a MR if there is only 1 S/G entry. That performance behavior
needs to default to disabled. The kernel must default to secure out of
the box.
> I will try to find some time to test the SRP changes in this series but I'm
> not sure yet when I will be able to do that.
This probably also takes care of the security issue for SRP, what do you
think?
>From d1ae6713fc011b6ff392e42cfb342899da8561ff Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
Date: Thu, 23 Jul 2015 12:19:53 -0600
Subject: [PATCH] IB/srp: Do not create an all physical insecure rkey by
default
The ULP only needs this if the insecure register_always performance
optimization is enabled, or if FRWR/FMR is not supported in the driver.
Do not create an all physical MR unless it is needed to support either
of those modes. Default register_always to true so the out of the box
configuration does not create an insecure all physical MR.
Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
---
drivers/infiniband/ulp/srp/ib_srp.c | 31 +++++++++++++++++++++----------
drivers/infiniband/ulp/srp/ib_srp.h | 2 +-
2 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index fb9fed0fac28..a1e3818d0791 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
static unsigned int indirect_sg_entries;
static bool allow_ext_sg;
static bool prefer_fr;
-static bool register_always;
+static bool register_always = true;
static int topspin_workarounds = 1;
module_param(srp_sg_tablesize, uint, 0444);
@@ -3150,7 +3150,8 @@ static ssize_t srp_create_target(struct device *dev,
target->scsi_host = target_host;
target->srp_host = host;
target->lkey = host->srp_dev->pd->local_dma_lkey;
- target->rkey = host->srp_dev->mr->rkey;
+ if (host->srp_dev->rkey_mr)
+ target->rkey = host->srp_dev->rkey_mr->rkey;
target->cmd_sg_cnt = cmd_sg_entries;
target->sg_tablesize = indirect_sg_entries ? : cmd_sg_entries;
target->allow_ext_sg = allow_ext_sg;
@@ -3381,6 +3382,7 @@ static void srp_add_one(struct ib_device *device)
struct srp_host *host;
int mr_page_shift, s, e, p;
u64 max_pages_per_mr;
+ unsigned int mr_flags = 0;
dev_attr = kmalloc(sizeof *dev_attr, GFP_KERNEL);
if (!dev_attr)
@@ -3399,8 +3401,11 @@ static void srp_add_one(struct ib_device *device)
device->map_phys_fmr && device->unmap_fmr);
srp_dev->has_fr = (dev_attr->device_cap_flags &
IB_DEVICE_MEM_MGT_EXTENSIONS);
- if (!srp_dev->has_fmr && !srp_dev->has_fr)
+ if (!srp_dev->has_fmr && !srp_dev->has_fr) {
dev_warn(&device->dev, "neither FMR nor FR is supported\n");
+ /* Fall back to using an insecure all physical rkey */
+ mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+ }
srp_dev->use_fast_reg = (srp_dev->has_fr &&
(!srp_dev->has_fmr || prefer_fr));
@@ -3436,12 +3441,17 @@ static void srp_add_one(struct ib_device *device)
if (IS_ERR(srp_dev->pd))
goto free_dev;
- srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
- IB_ACCESS_LOCAL_WRITE |
- IB_ACCESS_REMOTE_READ |
- IB_ACCESS_REMOTE_WRITE);
- if (IS_ERR(srp_dev->mr))
- goto err_pd;
+ if (register_always)
+ mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+
+ if (mr_flags) {
+ srp_dev->rkey_mr = ib_get_dma_mr(
+ srp_dev->pd, IB_ACCESS_LOCAL_WRITE | mr_flags);
+ if (IS_ERR(srp_dev->rkey_mr))
+ goto err_pd;
+ } else
+ srp_dev->rkey_mr = NULL;
+
if (device->node_type == RDMA_NODE_IB_SWITCH) {
s = 0;
@@ -3506,7 +3516,8 @@ static void srp_remove_one(struct ib_device *device)
kfree(host);
}
- ib_dereg_mr(srp_dev->mr);
+ if (srp_dev->rkey_mr)
+ ib_dereg_mr(srp_dev->rkey_mr);
ib_dealloc_pd(srp_dev->pd);
kfree(srp_dev);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 17ee3f80ba55..8b241f17f8b8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -95,7 +95,7 @@ struct srp_device {
struct list_head dev_list;
struct ib_device *dev;
struct ib_pd *pd;
- struct ib_mr *mr;
+ struct ib_mr *rkey_mr;
u64 mr_page_mask;
int mr_page_size;
int mr_max_size;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
