Re: scsi: ufs: Optimize host lock on transfer requests send/compl paths (uninitialized pointer error)

[Can Guo]

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@xxxxxxxxxxxxxx>
> 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.