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

Can Guo
2021-06-09T01:11:16Z

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.