linux-kernel - Re: [PATCH] scsi: target: cxgbit: check skb dequeue result

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98435c8c-8bdc-35be-4d81-3491f872f555@yandex-team.ru>
Date:   Mon, 10 Apr 2023 10:35:56 +0300
From:   Denis Plotnikov <den-plotnikov@...dex-team.ru>
To:     Bodo Stroesser <bostroesser@...il.com>, linux-scsi@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, target-devel@...r.kernel.org,
        varun@...lsio.com, nab@...ux-iscsi.org, martin.petersen@...cle.com
Subject: Re: [PATCH] scsi: target: cxgbit: check skb dequeue result


On 07.04.2023 22:16, Bodo Stroesser wrote:
> On 06.04.23 17:04, Denis Plotnikov wrote:
>> On a couple of abort packet paths skb dequeuing may end up with
>> returning NULL, which, in turn, may end up with further null
>> pointer dereference.
>>
>> Fix it by checking the return value of skb dequeuing.
>>
>> Found by Linux Verification Center(linuxtesting.org) with SVACE.
>>
>> Fixes: 9730ffcb8957 (cxgbit: add files for cxgbit.ko)
>> Signed-off-by: Denis Plotnikov <den-plotnikov@...dex-team.ru>
>> ---
>>   drivers/target/iscsi/cxgbit/cxgbit_cm.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/cxgbit/cxgbit_cm.c 
>> b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
>> index 518ded214e74..d43fd761c20a 100644
>> --- a/drivers/target/iscsi/cxgbit/cxgbit_cm.c
>> +++ b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
>> @@ -669,6 +669,9 @@ static int cxgbit_send_abort_req(struct 
>> cxgbit_sock *csk)
>>           cxgbit_send_tx_flowc_wr(csk);
>>         skb = __skb_dequeue(&csk->skbq);
>> +    if (!skb)
>> +        return 0;
>> +
>>       cxgb_mk_abort_req(skb, len, csk->tid, csk->txq_idx,
>>                 csk->com.cdev, cxgbit_abort_arp_failure);
>>   @@ -1769,9 +1772,10 @@ static void cxgbit_abort_req_rss(struct 
>> cxgbit_sock *csk, struct sk_buff *skb)
>>           cxgbit_send_tx_flowc_wr(csk);
>>         rpl_skb = __skb_dequeue(&csk->skbq);
>> -
>> -    cxgb_mk_abort_rpl(rpl_skb, len, csk->tid, csk->txq_idx);
>> -    cxgbit_ofld_send(csk->com.cdev, rpl_skb);
>> +    if (!rpl_skb) {
>
> Honestly I have no clue about cxgbit, but to avoid null pointer
> dereference, shouldn't it be "if (rpl_skb) {"?

Yes it should. Thanks for finding that. I'll resend the patch.

Frankly, I have no clue about cxgbit as well, so may be there is a 
better way to cope with the null pointer issue here.

Denis

>
> Bodo
>
>
>> +        cxgb_mk_abort_rpl(rpl_skb, len, csk->tid, csk->txq_idx);
>> +        cxgbit_ofld_send(csk->com.cdev, rpl_skb);
>> +    }
>>         if (wakeup_thread) {
>>           cxgbit_queue_rx_skb(csk, skb);