Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[Jens Axboe]

On 3/18/26 4:20 AM, ??? wrote:
> On 2/6/26 13:58 PM, ??? wrote:
>> On 2/6/26 00:45, Hannes Reinecke wrote:
>>> On 2/5/26 14:42, Jens Axboe wrote:
>>>> On 2/4/26 10:32 PM, ??? wrote:
>>>>> bsg: initialize request and reply payloads in bsg_prepare_job
>>>>>
>>>>> struct bsg_job payloads contain fields that are only populated by
>>>>> certain commands, such as sg_list pointers.
>>>>>
>>>>> Because struct bsg_job is allocated with kmalloc(), memory may be
>>>>> reused across requests. If a command does not populate all payload
>>>>> fields, stale state from a previous job may remain and later be
>>>>> misinterpreted during cleanup, potentially leading to use-after-free
>>>>> or double-free issues.
>>>>>
>>>>> Initialize both request and reply payloads at the beginning of job
>>>>> preparation to ensure a clean state for all commands.
>>>>>
>>>>> Signed-off-by: Jonghwi Rha 
>>>>>
>>>>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>>>>> index 32da4a4429ce..0fbf8e311c03 100644
>>>>> --- a/block/bsg-lib.c
>>>>> +++ b/block/bsg-lib.c
>>>>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>>>>          struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>>>>          int ret;
>>>>>
>>>>> +      /* Clear stale SG state since bsg_job is reused as a request PDU */
>>>>> +      job->request_payload.sg_list = NULL;
>>>>> +      job->request_payload.sg_cnt = 0;
>>>>> +      job->reply_payload.sg_list = NULL;
>>>>> +      job->reply_payload.sg_cnt = 0;
>>>>> +
>>>>>          job->timeout = req->timeout;
>>>>>
>>>>>          if (req->bio) {
>>>>
>>>> The patch is white-space damaged, tabs are spaces. But I can fix that
>>>> up. Do we just want to do a memset(job, 0, sizeof(*job)) here to avoid
>>>> any oddities like this in the future?
>>>>
>>>
>>> That might indeed be better.
>>
>> The suggested method impairs normal operation. If bsg_prepare_job performs
>> a zero?memset for the job structure, all request?related information set on
>> the driver side before the call will be lost. Therefore, if it runs as is,
>> it will go to ufs_bsg_request and cause a null?pointer access.
>>
>> Currently, the original patch has no functional impact.
>>
>> The blank problem seems to be due to a mistake I made while copying and pasting
>> the patch. I am reattaching the patch below. If needed, I can attach the patch
>> and resend the new email.
>>
>>
>> [PATCH] bsg: initialize request and reply payloads in bsg_prepare_job
>>
>> struct bsg_job payloads contain fields that are only populated by
>> certain commands, such as sg_list pointers.
>>
>> Because struct bsg_job is allocated with kmalloc(), memory may be
>> reused across requests. If a command does not populate all payload
>> fields, stale state from a previous job may remain and later be
>> misinterpreted during cleanup, potentially leading to use-after-free
>> or double-free issues.
>>
>> Initialize both request and reply payloads at the beginning of job
>> preparation to ensure a clean state for all commands.
>>
>> Signed-off-by: Jonghwi Rha 
>> ---
>>  block/bsg-lib.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>> index 32da4a4429ce..0fbf8e311c03 100644
>> --- a/block/bsg-lib.c
>> +++ b/block/bsg-lib.c
>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>  	struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>  	int ret;
>>  
>> +	/* Clear stale SG state since bsg_job is reused as a request PDU */
>> +	job->request_payload.sg_list = NULL;
>> +	job->request_payload.sg_cnt = 0;
>> +	job->reply_payload.sg_list = NULL;
>> +	job->reply_payload.sg_cnt = 0;
>> +
>> 	job->timeout = req->timeout;
>>
>> 	if (req->bio) {
>> -- 
> 
>> Regards,
>> Jonghwi,
> 
> --
> 
> Since there was no reply, I am resending the email as a reminder.
> First, I have confirmed in my environment that, as you suggested,
> memset?as 0 for all 'job' struct elements eventually results an error.
> The reason is, as I mentioned above, that the request/reply gets lost
> before re-using.
> 
> Also, since other elements in the structure are reused, so they are
> not relevant to the current issue.
> 
> If the code execution point is not ideal, there is also the option of
> zeroising after freeing the memory allocation.

Just send it out as a proper patch and we can take a look at it again.

-- 
Jens Axboe