| Message ID | 20250410001320.2219341-1-chenyuan0y@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] scsi: ufs: mcq: Add NULL check in ufshcd_mcq_abort() | expand |
On 4/9/25 5:13 PM, Chenyuan Yang wrote: > A race can occur between the MCQ completion path and the abort handler: > once a request completes, __blk_mq_free_request() sets rq->mq_hctx to > NULL, meaning the subsequent ufshcd_mcq_req_to_hwq() call in > ufshcd_mcq_abort() can return a NULL pointer. If this NULL pointer is > dereferenced, the kernel will crash. > > Add a NULL check for the returned hwq pointer. If hwq is NULL, log an > error and return FAILED, preventing a potential NULL-pointer dereference. > As suggested by Bart, the ufshcd_cmd_inflight() check is removed. > > This is similar to the fix in commit 74736103fb41 > ("scsi: ufs: core: Fix ufshcd_abort_one racing issue"). > > This is found by our static analysis tool KNighter. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Wed, 2025-04-09 at 19:13 -0500, Chenyuan Yang wrote: > > A race can occur between the MCQ completion path and the abort > handler: > once a request completes, __blk_mq_free_request() sets rq->mq_hctx to > NULL, meaning the subsequent ufshcd_mcq_req_to_hwq() call in > ufshcd_mcq_abort() can return a NULL pointer. If this NULL pointer is > dereferenced, the kernel will crash. > > Add a NULL check for the returned hwq pointer. If hwq is NULL, log an > error and return FAILED, preventing a potential NULL-pointer > dereference. > As suggested by Bart, the ufshcd_cmd_inflight() check is removed. > > This is similar to the fix in commit 74736103fb41 > ("scsi: ufs: core: Fix ufshcd_abort_one racing issue"). > > This is found by our static analysis tool KNighter. > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> > Fixes: f1304d442077 ("scsi: ufs: mcq: Added ufshcd_mcq_abort()") > --- > drivers/ufs/core/ufs-mcq.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 240ce135bbfb..f1294c29f484 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -677,13 +677,6 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) > unsigned long flags; > int err; > > - if (!ufshcd_cmd_inflight(lrbp->cmd)) { > - dev_err(hba->dev, > - "%s: skip abort. cmd at tag %d already > completed.\n", > - __func__, tag); > - return FAILED; > - } > - > /* Skip task abort in case previous aborts failed and report > failure */ > if (lrbp->req_abort_skip) { > dev_err(hba->dev, "%s: skip abort. tag %d failed > earlier\n", > @@ -692,6 +685,11 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) > } > > hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd)); > + if (!hwq) { > + dev_err(hba->dev, "%s: skip abort. cmd at tag %d > already completed.\n", > + __func__, tag); > + return FAILED; > + } > > if (ufshcd_mcq_sqe_search(hba, hwq, tag)) { > /* > -- > 2.34.1 > Reviewed-by: Peter Wang <peter.wang@mediatek.com>
On Wed, 09 Apr 2025 19:13:20 -0500, Chenyuan Yang wrote: > A race can occur between the MCQ completion path and the abort handler: > once a request completes, __blk_mq_free_request() sets rq->mq_hctx to > NULL, meaning the subsequent ufshcd_mcq_req_to_hwq() call in > ufshcd_mcq_abort() can return a NULL pointer. If this NULL pointer is > dereferenced, the kernel will crash. > > Add a NULL check for the returned hwq pointer. If hwq is NULL, log an > error and return FAILED, preventing a potential NULL-pointer dereference. > As suggested by Bart, the ufshcd_cmd_inflight() check is removed. > > [...] Applied to 6.15/scsi-fixes, thanks! [1/1] scsi: ufs: mcq: Add NULL check in ufshcd_mcq_abort() https://git.kernel.org/mkp/scsi/c/4c3240850629
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 240ce135bbfb..f1294c29f484 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -677,13 +677,6 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) unsigned long flags; int err; - if (!ufshcd_cmd_inflight(lrbp->cmd)) { - dev_err(hba->dev, - "%s: skip abort. cmd at tag %d already completed.\n", - __func__, tag); - return FAILED; - } - /* Skip task abort in case previous aborts failed and report failure */ if (lrbp->req_abort_skip) { dev_err(hba->dev, "%s: skip abort. tag %d failed earlier\n", @@ -692,6 +685,11 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) } hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd)); + if (!hwq) { + dev_err(hba->dev, "%s: skip abort. cmd at tag %d already completed.\n", + __func__, tag); + return FAILED; + } if (ufshcd_mcq_sqe_search(hba, hwq, tag)) { /*
A race can occur between the MCQ completion path and the abort handler: once a request completes, __blk_mq_free_request() sets rq->mq_hctx to NULL, meaning the subsequent ufshcd_mcq_req_to_hwq() call in ufshcd_mcq_abort() can return a NULL pointer. If this NULL pointer is dereferenced, the kernel will crash. Add a NULL check for the returned hwq pointer. If hwq is NULL, log an error and return FAILED, preventing a potential NULL-pointer dereference. As suggested by Bart, the ufshcd_cmd_inflight() check is removed. This is similar to the fix in commit 74736103fb41 ("scsi: ufs: core: Fix ufshcd_abort_one racing issue"). This is found by our static analysis tool KNighter. Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> Fixes: f1304d442077 ("scsi: ufs: mcq: Added ufshcd_mcq_abort()") --- drivers/ufs/core/ufs-mcq.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)