There's no need to get_random_bytes() into a temporary buffer.
This is not a no-brainer change; get_random_u32() has slightly weaker
security guarantees, but code like this is the classic example of when
it's appropriate: the random value is stored in the kernel for as long
as it's valuable.
TODO: Could any of the call sites be further weakened to prandom_u32()?
If we're randomizing to avoid collisions with a *cooperating* (as opposed
to malicious) partner, we don't need cryptographic strength.
Signed-off-by: George Spelvin <[email protected]>
Cc: Doug Ledford <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
Cc: Faisal Latif <[email protected]>
Cc: Shiraz Saleem <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Bernard Metzler <[email protected]>
---
drivers/infiniband/core/cm.c | 2 +-
drivers/infiniband/core/cma.c | 3 +--
drivers/infiniband/core/sa_query.c | 2 +-
drivers/infiniband/hw/i40iw/i40iw_verbs.c | 2 +-
drivers/infiniband/sw/siw/siw_mem.c | 9 ++-------
drivers/infiniband/sw/siw/siw_verbs.c | 2 +-
drivers/infiniband/ulp/srp/ib_srp.c | 3 +--
7 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 4decc1d4cc997..8af4368faf8ee 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4449,7 +4449,7 @@ static int __init ib_cm_init(void)
cm.remote_qp_table = RB_ROOT;
cm.remote_sidr_table = RB_ROOT;
xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
- get_random_bytes(&cm.random_id_operand, sizeof cm.random_id_operand);
+ cm.random_id_operand = (__force __be32)get_random_u32();
INIT_LIST_HEAD(&cm.timewait_list);
ret = class_register(&cm_class);
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index a5874d2fac54e..1fce71e625c15 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -873,9 +873,8 @@ struct rdma_cm_id *__rdma_create_id(struct net *net,
mutex_init(&id_priv->handler_mutex);
INIT_LIST_HEAD(&id_priv->listen_list);
INIT_LIST_HEAD(&id_priv->mc_list);
- get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
+ id_priv->seq_num = get_random_u32() & 0x00ffffff;
id_priv->id.route.addr.dev_addr.net = get_net(net);
- id_priv->seq_num &= 0x00ffffff;
return &id_priv->id;
}
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 30d4c126a2db0..0db882e247234 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -2426,7 +2426,7 @@ int ib_sa_init(void)
{
int ret;
- get_random_bytes(&tid, sizeof tid);
+ tid = get_random_u32();
atomic_set(&ib_nl_sa_request_seq, 0);
diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
index dbd96d029d8bd..4c62b3a4f4233 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
@@ -1262,7 +1262,7 @@ static u32 i40iw_create_stag(struct i40iw_device *iwdev)
u8 consumer_key;
int ret;
- get_random_bytes(&random, sizeof(random));
+ random = get_random_u32();
consumer_key = (u8)random;
driver_key = random & ~iwdev->mr_stagmask;
diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index e99983f076631..50c84f6a98e51 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -21,10 +21,7 @@
int siw_mem_add(struct siw_device *sdev, struct siw_mem *m)
{
struct xa_limit limit = XA_LIMIT(1, 0x00ffffff);
- u32 id, next;
-
- get_random_bytes(&next, 4);
- next &= 0x00ffffff;
+ u32 id, next = get_random_u32() & 0x00ffffff;
if (xa_alloc_cyclic(&sdev->mem_xa, &id, m, limit, &next,
GFP_KERNEL) < 0)
@@ -107,9 +104,7 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
kref_init(&mem->ref);
mr->mem = mem;
-
- get_random_bytes(&next, 4);
- next &= 0x00ffffff;
+ next = get_random_u32() & 0x00ffffff;
if (xa_alloc_cyclic(&sdev->mem_xa, &id, mem, limit, &next,
GFP_KERNEL) < 0) {
diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 5fd6d6499b3d7..42f3ced4ca7c7 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -1139,7 +1139,7 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr,
rv = -ENOMEM;
goto err_out;
}
- get_random_bytes(&cq->id, 4);
+ cq->id = get_random_u32();
siw_dbg(base_cq->device, "new CQ [%u]\n", cq->id);
spin_lock_init(&cq->lock);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index cd1181c39ed29..2a954db0d6b55 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -903,8 +903,7 @@ static int srp_send_req(struct srp_rdma_ch *ch, uint32_t max_iu_len,
req->ib_param.primary_path = &ch->ib_cm.path;
req->ib_param.alternate_path = NULL;
req->ib_param.service_id = target->ib_cm.service_id;
- get_random_bytes(&req->ib_param.starting_psn, 4);
- req->ib_param.starting_psn &= 0xffffff;
+ req->ib_param.starting_psn = get_random_u32() & 0xffffff;
req->ib_param.qp_num = ch->qp->qp_num;
req->ib_param.qp_type = ch->qp->qp_type;
req->ib_param.local_cm_response_timeout = subnet_timeout + 2;
--
2.26.0
On Wed, Aug 21, 2019 at 08:21:45PM -0400, George Spelvin wrote:
> There's no need to get_random_bytes() into a temporary buffer.
>
> This is not a no-brainer change; get_random_u32() has slightly weaker
> security guarantees, but code like this is the classic example of when
> it's appropriate: the random value is stored in the kernel for as long
> as it's valuable.
The mechanical transformation looks OK, but can someone who knows the
RNG confirm this statement?
Many of these places are being used in network related contexts, I
suspect the value here is often less about secrecy, more about
unguessability.
Jason
-----"George Spelvin" <[email protected]> wrote: -----
>To: [email protected], [email protected]
>From: "George Spelvin" <[email protected]>
>Date: 03/28/2020 05:43PM
>Cc: "Doug Ledford" <[email protected]>, "Jason Gunthorpe"
><[email protected]>, [email protected], "Faisal Latif"
><[email protected]>, "Shiraz Saleem" <[email protected]>,
>"Bart Van Assche" <[email protected]>, "Bernard Metzler"
><[email protected]>
>Subject: [EXTERNAL] [RFC PATCH v1 42/50] drivers/ininiband: Use
>get_random_u32()
>
>There's no need to get_random_bytes() into a temporary buffer.
>
>This is not a no-brainer change; get_random_u32() has slightly weaker
>security guarantees, but code like this is the classic example of
>when
>it's appropriate: the random value is stored in the kernel for as
>long
>as it's valuable.
>
>TODO: Could any of the call sites be further weakened to
>prandom_u32()?
>If we're randomizing to avoid collisions with a *cooperating* (as
>opposed
>to malicious) partner, we don't need cryptographic strength.
>
>Signed-off-by: George Spelvin <[email protected]>
>Cc: Doug Ledford <[email protected]>
>Cc: Jason Gunthorpe <[email protected]>
>Cc: [email protected]
>Cc: Faisal Latif <[email protected]>
>Cc: Shiraz Saleem <[email protected]>
>Cc: Bart Van Assche <[email protected]>
>Cc: Bernard Metzler <[email protected]>
>---
> drivers/infiniband/core/cm.c | 2 +-
> drivers/infiniband/core/cma.c | 3 +--
> drivers/infiniband/core/sa_query.c | 2 +-
> drivers/infiniband/hw/i40iw/i40iw_verbs.c | 2 +-
> drivers/infiniband/sw/siw/siw_mem.c | 9 ++-------
> drivers/infiniband/sw/siw/siw_verbs.c | 2 +-
> drivers/infiniband/ulp/srp/ib_srp.c | 3 +--
> 7 files changed, 8 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/infiniband/core/cm.c
>b/drivers/infiniband/core/cm.c
>index 4decc1d4cc997..8af4368faf8ee 100644
>--- a/drivers/infiniband/core/cm.c
>+++ b/drivers/infiniband/core/cm.c
>@@ -4449,7 +4449,7 @@ static int __init ib_cm_init(void)
> cm.remote_qp_table = RB_ROOT;
> cm.remote_sidr_table = RB_ROOT;
> xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC |
>XA_FLAGS_LOCK_IRQ);
>- get_random_bytes(&cm.random_id_operand, sizeof
>cm.random_id_operand);
>+ cm.random_id_operand = (__force __be32)get_random_u32();
> INIT_LIST_HEAD(&cm.timewait_list);
>
> ret = class_register(&cm_class);
>diff --git a/drivers/infiniband/core/cma.c
>b/drivers/infiniband/core/cma.c
>index a5874d2fac54e..1fce71e625c15 100644
>--- a/drivers/infiniband/core/cma.c
>+++ b/drivers/infiniband/core/cma.c
>@@ -873,9 +873,8 @@ struct rdma_cm_id *__rdma_create_id(struct net
>*net,
> mutex_init(&id_priv->handler_mutex);
> INIT_LIST_HEAD(&id_priv->listen_list);
> INIT_LIST_HEAD(&id_priv->mc_list);
>- get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
>+ id_priv->seq_num = get_random_u32() & 0x00ffffff;
> id_priv->id.route.addr.dev_addr.net = get_net(net);
>- id_priv->seq_num &= 0x00ffffff;
>
> return &id_priv->id;
> }
>diff --git a/drivers/infiniband/core/sa_query.c
>b/drivers/infiniband/core/sa_query.c
>index 30d4c126a2db0..0db882e247234 100644
>--- a/drivers/infiniband/core/sa_query.c
>+++ b/drivers/infiniband/core/sa_query.c
>@@ -2426,7 +2426,7 @@ int ib_sa_init(void)
> {
> int ret;
>
>- get_random_bytes(&tid, sizeof tid);
>+ tid = get_random_u32();
>
> atomic_set(&ib_nl_sa_request_seq, 0);
>
>diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>index dbd96d029d8bd..4c62b3a4f4233 100644
>--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>@@ -1262,7 +1262,7 @@ static u32 i40iw_create_stag(struct
>i40iw_device *iwdev)
> u8 consumer_key;
> int ret;
>
>- get_random_bytes(&random, sizeof(random));
>+ random = get_random_u32();
> consumer_key = (u8)random;
>
> driver_key = random & ~iwdev->mr_stagmask;
>diff --git a/drivers/infiniband/sw/siw/siw_mem.c
>b/drivers/infiniband/sw/siw/siw_mem.c
>index e99983f076631..50c84f6a98e51 100644
>--- a/drivers/infiniband/sw/siw/siw_mem.c
>+++ b/drivers/infiniband/sw/siw/siw_mem.c
>@@ -21,10 +21,7 @@
> int siw_mem_add(struct siw_device *sdev, struct siw_mem *m)
> {
> struct xa_limit limit = XA_LIMIT(1, 0x00ffffff);
>- u32 id, next;
>-
>- get_random_bytes(&next, 4);
>- next &= 0x00ffffff;
>+ u32 id, next = get_random_u32() & 0x00ffffff;
>
> if (xa_alloc_cyclic(&sdev->mem_xa, &id, m, limit, &next,
> GFP_KERNEL) < 0)
>@@ -107,9 +104,7 @@ int siw_mr_add_mem(struct siw_mr *mr, struct
>ib_pd *pd, void *mem_obj,
> kref_init(&mem->ref);
>
> mr->mem = mem;
>-
>- get_random_bytes(&next, 4);
>- next &= 0x00ffffff;
>+ next = get_random_u32() & 0x00ffffff;
>
> if (xa_alloc_cyclic(&sdev->mem_xa, &id, mem, limit, &next,
> GFP_KERNEL) < 0) {
>diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>b/drivers/infiniband/sw/siw/siw_verbs.c
>index 5fd6d6499b3d7..42f3ced4ca7c7 100644
>--- a/drivers/infiniband/sw/siw/siw_verbs.c
>+++ b/drivers/infiniband/sw/siw/siw_verbs.c
>@@ -1139,7 +1139,7 @@ int siw_create_cq(struct ib_cq *base_cq, const
>struct ib_cq_init_attr *attr,
> rv = -ENOMEM;
> goto err_out;
> }
>- get_random_bytes(&cq->id, 4);
>+ cq->id = get_random_u32();
> siw_dbg(base_cq->device, "new CQ [%u]\n", cq->id);
>
> spin_lock_init(&cq->lock);
>diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>b/drivers/infiniband/ulp/srp/ib_srp.c
>index cd1181c39ed29..2a954db0d6b55 100644
>--- a/drivers/infiniband/ulp/srp/ib_srp.c
>+++ b/drivers/infiniband/ulp/srp/ib_srp.c
>@@ -903,8 +903,7 @@ static int srp_send_req(struct srp_rdma_ch *ch,
>uint32_t max_iu_len,
> req->ib_param.primary_path = &ch->ib_cm.path;
> req->ib_param.alternate_path = NULL;
> req->ib_param.service_id = target->ib_cm.service_id;
>- get_random_bytes(&req->ib_param.starting_psn, 4);
>- req->ib_param.starting_psn &= 0xffffff;
>+ req->ib_param.starting_psn = get_random_u32() & 0xffffff;
> req->ib_param.qp_num = ch->qp->qp_num;
> req->ib_param.qp_type = ch->qp->qp_type;
> req->ib_param.local_cm_response_timeout = subnet_timeout + 2;
>--
>2.26.0
>
>
Speaking for the siw driver only, these two changes are looking
good to me. What is needed is a pseudo-random number, not
to easy to guess for the application. get_random_u32() provides that.
Thanks!
Reviewed-by: Bernard Metzler <[email protected]>
On Sun, Mar 29, 2020 at 11:36:21AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 08:21:45PM -0400, George Spelvin wrote:
>> There's no need to get_random_bytes() into a temporary buffer.
>>
>> This is not a no-brainer change; get_random_u32() has slightly weaker
>> security guarantees, but code like this is the classic example of when
>> it's appropriate: the random value is stored in the kernel for as long
>> as it's valuable.
>
> The mechanical transformation looks OK, but can someone who knows the
> RNG confirm this statement?
You might find commit 92e507d21613 ("random: document get_random_int()
family") informative.
> Many of these places are being used in network related contexts, I
> suspect the value here is often less about secrecy, more about
> unguessability.
The significant difference is backtrackability. Each get_random_bytes()
call has a final anti-backtracking step, to ensure that the random number
just generated cannot be recovered from the subsequent kernel state.
This is appropriate for encryption keys or asymmetric keys.
The get_random_{int,long,u32,u64} functions omit this step, which means
they only need one ChaCha20 crypto operation per 64 bytes of output,
not a minimum of one per call.
One good way of distinguishing the cases is to look for calls to
memzero_explicit(). If you need to ensure the random bytes are securely
destroyed, you need antibacktracking. If your application doesn't care if
anyone learns your session authentication keys after the session has been
closed, you don't need it.
On Sun, Mar 29, 2020 at 03:01:36PM +0000, Bernard Metzler wrote:
> -----"George Spelvin" <[email protected]> wrote: -----
>> Subject: [EXTERNAL] [RFC PATCH v1 42/50] drivers/ininiband: Use
get_random_u32()
>>
>> There's no need to get_random_bytes() into a temporary buffer.
>>
>> This is not a no-brainer change; get_random_u32() has slightly weaker
>> security guarantees, but code like this is the classic example of when
>> it's appropriate: the random value is stored in the kernel for as long
>> as it's valuable.
>>
>> TODO: Could any of the call sites be further weakened to prandom_u32()?
>> If we're randomizing to avoid collisions with a *cooperating* (as opposed
>> to malicious) partner, we don't need cryptographic strength.
>>
>> Signed-off-by: George Spelvin <[email protected]>
>> Cc: Doug Ledford <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Cc: [email protected]
>> Cc: Faisal Latif <[email protected]>
>> Cc: Shiraz Saleem <[email protected]>
>> Cc: Bart Van Assche <[email protected]>
>> Cc: Bernard Metzler <[email protected]>
>> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>> b/drivers/infiniband/sw/siw/siw_verbs.c
>> index 5fd6d6499b3d7..42f3ced4ca7c7 100644
>> --- a/drivers/infiniband/sw/siw/siw_verbs.c
>> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
>> @@ -1139,7 +1139,7 @@ int siw_create_cq(struct ib_cq *base_cq, const
>> struct ib_cq_init_attr *attr,
>> rv = -ENOMEM;
>> goto err_out;
>> }
>> - get_random_bytes(&cq->id, 4);
>> + cq->id = get_random_u32();
>> siw_dbg(base_cq->device, "new CQ [%u]\n", cq->id);
>>
>> spin_lock_init(&cq->lock);
> Speaking for the siw driver only, these two changes are looking
> good to me. What is needed is a pseudo-random number, not
> to easy to guess for the application. get_random_u32() provides that.
>
> Thanks!
>
> Reviewed-by: Bernard Metzler <[email protected]>
Just so you know, get_random_u32() is still crypto-strength: it is
unguessable even to a resourceful attacker with access to large amounts
of other get_random_u32() output.
prandom_u32() is much cheaper, but although well seeded and distributed
(so equally resistant to accidental collisions), *is* guessable if someone
really wants to work at it.
Many intra-machine networks (like infiniband) are specifically not
designed to be robust in the face of malicious actors on the network.
A random transaction ID is sent in the clear, and a malicious actor
wanting to interfere could simply copy it.
In such cases, there's no need for crypto-grade numbers, because the
network already assumes that nobody's actively trying to create
collisions.
You seem to be saying that the siw driver could use prandom_u32().
On Sun, Mar 29, 2020 at 04:52:04PM +0000, George Spelvin wrote:
> Many intra-machine networks (like infiniband) are specifically not
> designed to be robust in the face of malicious actors on the network.
This is not really true at all..
Jason
On Sun, Mar 29, 2020 at 05:02:13PM -0300, Jason Gunthorpe wrote:
> On Sun, Mar 29, 2020 at 04:52:04PM +0000, George Spelvin wrote:
>> Many intra-machine networks (like infiniband) are specifically not
>> designed to be robust in the face of malicious actors on the network.
>
> This is not really true at all..
Eep, this came out wrong! Let me clarify:
Many intra-machine networks like SCSI, LPC, HyperTransport, QuickPath,
and I2C are specifically not designed to be robust in the face of malicious
actors on the network/bus.
I don't know, *and was wondering*, whether this is true of Infiniband.
Does that make more sense?