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

From: Can Guo
Date: Tue Jun 08 2021 - 21:11:17 EST


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.