| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c7748a2b-2c27-d171-810c-f3eb56120944@gmail.com>
Date: Fri, 13 Jan 2023 16:47:07 +0800
From: Jia-Ju Bai <baijiaju1990@...il.com>
To: Zhu Yanjun <yanjun.zhu@...ux.dev>, zyjzyj2000@...il.com,
jgg@...pe.ca, leon@...nel.org
Cc: linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
TOTE Robot <oslab@...nghua.edu.cn>
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr
Thanks for the reply :)
I checked the commit 3282a549cf9b, and it said:
"If responder get a zero-byte RDMA Read request, qp->resp.mr is not set
in check_rkey() (see IBA C9-88)."
Thus, this commit added a NULL check for mr (aliased with qp->resp.mr)
in read_reply().
The buggy call trace of the commit 3282a549cf9b is rxe_responder() ->
read_reply(qp).
In the code, there are some possible call traces from rxe_responder():
rxe_responder()
execute(qp)
write_data_in(qp)
err = rxe_mr_copy(qp->resp.mr)
rxe_responder()
process_flush(qp)
mr = qp->resp.mr
start = mr->ibmr.iova;
length = mr->ibmr.length
rxe_flush_pmem_iova(mr)
rxe_responder()
atomic_reply(qp)
mr = qp->resp.mr
if (mr->state != RXE_MR_STATE_VALID)
rxe_responder()
atomic_write_reply(qp)
do_atomic_write(qp)
mr = qp->resp.mr
if (mr->state != RXE_MR_STATE_VALID)
dst = iova_to_vaddr(mr)
rxe_responder()
read_reply(qp)
mr = qp->resp.mr
err = rxe_mr_copy(mr)
For these possible call traces, they may share the same qp->resp.mr in
the commit 3282a549cf9b.
Thus, qp->resp.mr should be checked in these call traces.
I am not quite sure of this, so I am looking forward to your opinions,
thanks :)
Best wishes,
Jia-Ju Bai
On 2023/1/13 15:53, Zhu Yanjun wrote:
> 在 2023/1/13 10:35, Jia-Ju Bai 写道:
>> In a previous commit 3282a549cf9b, qp->resp.mr could be NULL. Moreover,
>> in many functions, qp->resp.mr is checked before its dereferences.
>> However, in some functions, this variable is not checked, and thus NULL
>> checks should be added.
>
> IMO， we should analyze the code snippet one by one.
> And it is not good to add "NULL check" without futher investigations.
>
> Zhu Yanjun
>>
>> These results are reported by a static tool written by myself.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@...il.com>
>> Reported-by: TOTE Robot <oslab@...nghua.edu.cn>
>> ---
>> drivers/infiniband/sw/rxe/rxe_resp.c | 47 ++++++++++++++++------------
>> 1 file changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c
>> b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index c74972244f08..2eafa1667a9e 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -621,11 +621,13 @@ static enum resp_states write_data_in(struct
>> rxe_qp *qp,
>> int err;
>> int data_len = payload_size(pkt);
>> - err = rxe_mr_copy(qp->resp.mr, qp->resp.va + qp->resp.offset,
>> - payload_addr(pkt), data_len, RXE_TO_MR_OBJ);
>> - if (err) {
>> - rc = RESPST_ERR_RKEY_VIOLATION;
>> - goto out;
>> + if (qp->resp.mr) {
>> + err = rxe_mr_copy(qp->resp.mr, qp->resp.va + qp->resp.offset,
>> + payload_addr(pkt), data_len, RXE_TO_MR_OBJ);
>> + if (err) {
>> + rc = RESPST_ERR_RKEY_VIOLATION;
>> + goto out;
>> + }
>> }
>> qp->resp.va += data_len;
>> @@ -699,11 +701,13 @@ static enum resp_states process_flush(struct
>> rxe_qp *qp,
>> start = res->flush.va;
>> length = res->flush.length;
>> } else { /* level == IB_FLUSH_MR */
>> - start = mr->ibmr.iova;
>> - length = mr->ibmr.length;
>> + if (mr) {
>> + start = mr->ibmr.iova;
>> + length = mr->ibmr.length;
>> + }
>> }
>> - if (res->flush.type & IB_FLUSH_PERSISTENT) {
>> + if (mr && res->flush.type & IB_FLUSH_PERSISTENT) {
>> if (rxe_flush_pmem_iova(mr, start, length))
>> return RESPST_ERR_RKEY_VIOLATION;
>> /* Make data persistent. */
>> @@ -742,7 +746,7 @@ static enum resp_states atomic_reply(struct
>> rxe_qp *qp,
>> qp->resp.res = res;
>> }
>> - if (!res->replay) {
>> + if (!res->replay && mr) {
>> if (mr->state != RXE_MR_STATE_VALID) {
>> ret = RESPST_ERR_RKEY_VIOLATION;
>> goto out;
>> @@ -793,15 +797,17 @@ static enum resp_states do_atomic_write(struct
>> rxe_qp *qp,
>> int payload = payload_size(pkt);
>> u64 src, *dst;
>> - if (mr->state != RXE_MR_STATE_VALID)
>> + if (mr && mr->state != RXE_MR_STATE_VALID)
>> return RESPST_ERR_RKEY_VIOLATION;
>> memcpy(&src, payload_addr(pkt), payload);
>> - dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
>> - /* check vaddr is 8 bytes aligned. */
>> - if (!dst || (uintptr_t)dst & 7)
>> - return RESPST_ERR_MISALIGNED_ATOMIC;
>> + if (mr) {
>> + dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset,
>> payload);
>> + /* check vaddr is 8 bytes aligned. */
>> + if (!dst || (uintptr_t)dst & 7)
>> + return RESPST_ERR_MISALIGNED_ATOMIC;
>> + }
>> /* Do atomic write after all prior operations have completed */
>> smp_store_release(dst, src);
>> @@ -1002,13 +1008,14 @@ static enum resp_states read_reply(struct
>> rxe_qp *qp,
>> return RESPST_ERR_RNR;
>> }
>> - err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
>> - payload, RXE_FROM_MR_OBJ);
>> - if (mr)
>> + if (mr) {
>> + err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
>> + payload, RXE_FROM_MR_OBJ);
>> rxe_put(mr);
>> - if (err) {
>> - kfree_skb(skb);
>> - return RESPST_ERR_RKEY_VIOLATION;
>> + if (err) {
>> + kfree_skb(skb);
>> + return RESPST_ERR_RKEY_VIOLATION;
>> + }
>> }
>> if (bth_pad(&ack_pkt)) {
>
Powered by blists - more mailing lists