Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <98435c8c-8bdc-35be-4d81-3491f872f555@yandex-team.ru>
Date:   Mon, 10 Apr 2023 10:35:56 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH] scsi: target: cxgbit: check skb dequeue result
To:     Bodo Stroesser <bostroesser@gmail.com>, linux-scsi@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, target-devel@vger.kernel.org,
        varun@chelsio.com, nab@linux-iscsi.org, martin.petersen@oracle.com
References: <20230406150405.300909-1-den-plotnikov@yandex-team.ru>
 <2170fe67-2e65-630b-22dc-a8f6d3f7966f@gmail.com>
Content-Language: en-US
From:   Denis Plotnikov <den-plotnikov@yandex-team.ru>
In-Reply-To: <2170fe67-2e65-630b-22dc-a8f6d3f7966f@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@yandex-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);