linux-kernel - Re: scsi: iscsi: Drop suspend calls from ep

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f2679a9b-7ccf-b4b8-c028-1c164d532fee@oracle.com>
Date:   Thu, 3 Jun 2021 18:27:38 -0500
From:   Mike Christie <michael.christie@...cle.com>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Lee Duncan <lduncan@...e.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Manish Rangankar <mrangankar@...vell.com>
Subject: Re: scsi: iscsi: Drop suspend calls from ep_disconnect

On 6/3/21 6:25 PM, Mike Christie wrote:
> On 6/3/21 5:25 PM, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis on linux-next with Coverity has found an issue in
>> drivers/scsi/qedi/qedi_iscsi.c with the following commit:
>>
>> commit 27e986289e739d08c1a4861cc3d3ec9b3a60845e
>> Author: Mike Christie <michael.christie@...cle.com>
>> Date:   Tue May 25 13:17:56 2021 -0500
>>
>>     scsi: iscsi: Drop suspend calls from ep_disconnect
>>
>> The analysis is as follows:
>>
>> 1662 void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess)
>> 1663 {
>> 1664        struct iscsi_session *session = cls_sess->dd_data;
>> 1665        struct iscsi_conn *conn = session->leadconn;
>>
>>     deref_ptr: Directly dereferencing pointer conn.
>>
>> 1666        struct qedi_conn *qedi_conn = conn->dd_data;
>> 1667
>> 1668        if (iscsi_is_session_online(cls_sess)) {
>>    Dereference before null check (REVERSE_INULL)
>>    check_after_deref: Null-checking conn suggests that it may be null,
>> but it has already been dereferenced on all paths leading to the check.
>>
>> 1669                if (conn)
>> 1670                        iscsi_suspend_queue(conn);
>> 1671                qedi_ep_disconnect(qedi_conn->iscsi_ep);
>> 1672        }
>>
>> Pointer conn is being checked to see if it is null, but earlier it has
>> been dereferenced on the assignment of qedi_conn.  So either conn will
>> be null at some point and a null ptr dereference occurs when qedi_conn
>> is assigned, or conn can never be null and the conn null check is
>> redundant and can be removed.
> 
> The analysis is correct.
> 
> The bigger problem is that this entire function seems racey with the
> normal conn/ep disconnect or shutdown.
> 
> Manish, when this function is run iscsid or the in-kernel conn error
> cleanup handler can be running right? There is nothing preventing
> those from running at the same time?
> 
> I think you want to call iscsi_host_remove at the beginning of __qedi_remove.
> That will tell userpsace that the host is being removed and libiscsi will

I meant iscsid not libiscsi.

> start the session shutdown and removal process. It then waits for the
> sessions to be removed. We can then proceed with the other host removal
> cleanup, and at the end of __qedi_remove you do the iscsi_host_free
> call.
>