Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: cang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 09 Jun 2021 09:10:38 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Colin Ian King <colin.king@canonical.com>
Cc:     Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: scsi: ufs: Optimize host lock on transfer requests send/compl
 paths (uninitialized pointer error)
In-Reply-To: <fa66c94c-3df6-3813-dc2d-572cee16071b@canonical.com>
References: <fa66c94c-3df6-3813-dc2d-572cee16071b@canonical.com>
Message-ID: <c723f68d3bfd535ea0c749fc93d06f32@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

Hi Colin,

On 2021-06-08 23:44, Colin Ian King wrote:
> Hi,
> 
> static analysis with Coverity on linux-next has found an issue in
> drivers/scsi/ufs/ufshcd.c introduced by the following commit:
> 
> commit a45f937110fa6b0c2c06a5d3ef026963a5759050
> Author: Can Guo <cang@codeaurora.org>
> Date:   Mon May 24 01:36:57 2021 -0700
> 
>     scsi: ufs: Optimize host lock on transfer requests send/compl paths
> 
> The analysis is as follows:
> 
> 
> 2948 static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
> 2949                enum dev_cmd_type cmd_type, int timeout)
> 2950 {
> 2951        struct request_queue *q = hba->cmd_queue;
> 2952        struct request *req;
> 
>     1. var_decl: Declaring variable lrbp without initializer.
> 
> 2953        struct ufshcd_lrb *lrbp;
> 2954        int err;
> 2955        int tag;
> 2956        struct completion wait;
> 2957
> 2958        down_read(&hba->clk_scaling_lock);
> 2959
> 2960        /*
> 2961         * Get free slot, sleep if slots are unavailable.
> 2962         * Even though we use wait_event() which sleeps 
> indefinitely,
> 2963         * the maximum wait time is bounded by SCSI request 
> timeout.
> 2964         */
> 2965        req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> 
>     2. Condition IS_ERR(req), taking false branch.
> 
> 2966        if (IS_ERR(req)) {
> 2967                err = PTR_ERR(req);
> 2968                goto out_unlock;
> 2969        }
> 2970        tag = req->tag;
> 
>     3. Condition !!__ret_warn_on, taking false branch.
>     4. Condition !!__ret_warn_on, taking false branch.
> 
> 2971        WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
> 2972        /* Set the timeout such that the SCSI error handler is not
> activated. */
> 2973        req->timeout = msecs_to_jiffies(2 * timeout);
> 2974        blk_mq_start_request(req);
> 2975
> 
>     5. Condition !!test_bit(tag, &hba->outstanding_reqs), taking true
> branch.
> 
> 2976        if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
> 2977                err = -EBUSY;
> 
>     6. Jumping to label out.
> 
> 2978                goto out;
> 2979        }
> 2980
> 2981        init_completion(&wait);
> 2982        lrbp = &hba->lrb[tag];
> 2983        WARN_ON(lrbp->cmd);
> 2984        err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> 2985        if (unlikely(err))
> 2986                goto out_put_tag;
> 2987
> 2988        hba->dev_cmd.complete = &wait;
> 2989
> 2990        ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND,
> lrbp->ucd_req_ptr);
> 2991        /* Make sure descriptors are ready before ringing the
> doorbell */
> 2992        wmb();
> 2993
> 2994        ufshcd_send_command(hba, tag);
> 2995        err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> 2996 out:
> 
>     7. Condition err, taking true branch.
> 
>     Uninitialized pointer read (UNINIT)
>     8. uninit_use: Using uninitialized value lrbp.
> 
> 2997        ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
> 2998                                    (struct utp_upiu_req
> *)lrbp->ucd_rsp_ptr);
> 2999
> 3000 out_put_tag:
> 3001        blk_put_request(req);
> 3002 out_unlock:
> 3003        up_read(&hba->clk_scaling_lock);
> 3004        return err;
> 3005 }
> 
> Pointer lrbp is being accessed on the error exit path on line 2989
> because it is no longer being initialized early, the pointer assignment
> was moved to a later point (line 2982) by the commit referenced in the
> top of the email.
> 
> Colin

I will fix it by changing "goto out;" -> "goto out_put_tag;" on line 
#2978
in a new patch.

Thanks,
Can Guo.