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

* re: scsi: ufs: Optimize host lock on transfer requests send/compl paths (uninitialized pointer error)
@ 2021-06-08 15:44 Colin Ian King
  2021-06-09  1:10 ` Can Guo
  0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2021-06-08 15:44 UTC (permalink / raw)
  To: Can Guo
  Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Matthias Brugger, linux-scsi, linux-kernel

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

^ permalink raw reply	[flat|nested] 2+ messages in thread