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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <08f664af-30be-aa4b-aa82-2333650dee06@oracle.com>
Date:   Thu, 3 Jun 2021 18:25:37 -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 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
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.