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 target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html