Re: [PATCH 1/3] scsi: ufs: bsg: Validate request_len before accessing request

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ufs/bsg: heap buffer overflow in Advanced RPMB reply handling
- [Medium] block: memory and device refcount leak on ufs_bsg_request() error path
--

commit 6c7ab99a857f0be8f1f19eb78aa230d8ccb39b14
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: bsg: Validate request_len before accessing request

This commit adds validation checks for job->request_len in the UFS
BSG paths to prevent out-of-bounds reads when userspace provides
short requests.

> diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
> index 7a66917b3fc0e..dab1f05aff383 100644
> --- a/drivers/ufs/core/ufs_bsg.c
> +++ b/drivers/ufs/core/ufs_bsg.c
> @@ -73,6 +73,9 @@ static int ufs_bsg_exec_advanced_rpmb_req(struct ufs_hba *hba, struct bsg_job *j
>  	if (hba->ufs_version < ufshci_version(4, 0) || !hba->dev_info.b_advanced_rpmb_en)
>  		return -EINVAL;
>  
> +	if (job->request_len < sizeof(*rpmb_request))
> +		return -EINVAL;
> +

[Severity: High]
While this properly validates the request size, does the reply handling
contain a heap buffer overflow for advanced RPMB requests?

The BSG core allocates job->reply in bsg_init_rq() with a fixed size of
SCSI_SENSE_BUFFERSIZE (96 bytes). However, ufs_bsg_exec_advanced_rpmb_req()
casts job->reply to struct ufs_rpmb_reply, which has a size of 104 bytes:
sizeof(struct ufs_bsg_reply) (40 bytes) + sizeof(struct ufs_ehs) (64 bytes).

When ufshcd_advanced_rpmb_req_handler() is called, it writes 64 bytes
to the ehs_rsp field at offset 40, resulting in an 8-byte out-of-bounds
write on the heap:

drivers/ufs/core/ufshcd.c:ufshcd_advanced_rpmb_req_handler() {
    ...
    memcpy(rsp_ehs, ehs_data, ehs_len * 32);
    ...
}

Later, ufs_bsg_request() sets job->reply_len to 104 for RPMB replies,
which configures the block layer to copy 104 bytes to user space from
this 96-byte allocation. Will this leak adjacent heap memory to userspace?

[ ... ]

> @@ -142,6 +145,11 @@ static int ufs_bsg_request(struct bsg_job *job)
>  
>  	bsg_reply->reply_payload_rcv_len = 0;
>  
> +	if (job->request_len < sizeof(*bsg_request)) {
> +		bsg_reply->result = -EINVAL;
> +		return -EINVAL;
> +	}
> +

[Severity: Medium]
Does returning -EINVAL directly bypass the BSG job completion mechanism
and permanently leak the job's resources?

When ufs_bsg_request() returns -EINVAL directly, the block layer's
bsg_queue_rq() returns BLK_STS_IOERR. This causes blk_mq_end_request()
to complete the request without invoking the queue's .complete callback
(bsg_complete()).

Because bsg_complete() is bypassed, bsg_job_put() is never called,
meaning bsg_teardown_job() never executes. This leaves the sg_list
memory and the job->dev reference count permanently leaked, which would
prevent the UFS device from ever being cleanly removed.

Should this error path instead call bsg_job_done(job, -EINVAL, 0)
and return 0 to ensure proper teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013102913.948718-1-stanleyjhu@google.com?part=1