RE: Re: [Samsung] bsg-lib.c patch for double-free error fix.

Posted by 라종휘 1 month, 4 weeks ago

Dear Jens,

Sorry for the excuse, also thank you for the response. 

It was my first time with the procedure, so there was a mistake. I apologize for the inconvenience.
As you mentioned, in the case of the previous patch, there was duplicate code, so changing it to kzalloc would indeed be meaningless.

We have prepared a new patch and completed internal testing on our side. Without this patch, issues occur when using ufs-bsg.

Starting from Linux kernel version 5.x, advanced RPMB code was included to the ufs-bsg path. In the advanced RPMB code path, the payload’s sg_list is not used. 
So, after other BSG operations, the previous value remains in payload.sg_list, which results in a double-free issue.

Author: Jonghwi Rha <jonghwi.rha@samsung.com>
Date:   Tue Jan 13 14:42:39 2026 +0900

    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 <jonghwi.rha@samsung.com>

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) {

P.S. Change-Id was for our gerrit system. Plz ignore it.

Regards,
Jonghwi,

---------Original Message---------
Sender: Jens Axboe <axboe@kernel.dk>
Date: 2026-01-13 00:36 (GMT+09:00)
Title: Re: [Samsung] bsg-lib.c patch for double-free error fix.
 
Please don't send patches as attachments, and particularly with html
emails as they will just get dropped from the list. And it makes it
impossible to reply to as well, as you then need to save and read the
patch separately and import it into an email...

> Change-Id: Iadb96f8736f8d9d9aae7b4a831c2a286ff59c520

What is this?

diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 9ceb5d0832f5..635b3b988f92 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -215,7 +215,7 @@ static int bsg_map_buffer(struct bsg_buffer *buf, struct request *req)

         BUG_ON(!req->nr_phys_segments);

-        buf->sg_list = kmalloc(sz, GFP_KERNEL);
+        buf->sg_list = kzalloc(sz, GFP_KERNEL);
         if (!buf->sg_list)
                 return -ENOMEM;
         sg_init_table(buf->sg_list, req->nr_phys_segments);

How does this make a difference, when sg_init_table() explicitly sets it
all to 0?

--
Jens Axboe

Re: [Samsung] bsg-lib.c patch for double-free error fix.

Posted by Jens Axboe 1 month, 3 weeks ago

On 2/2/26 5:04 AM, ??? wrote:
> Dear Jens,
> 
> Sorry for the excuse, also thank you for the response. 
> 
> It was my first time with the procedure, so there was a mistake. I
> apologize for the inconvenience. As you mentioned, in the case of the
> previous patch, there was duplicate code, so changing it to kzalloc
> would indeed be meaningless.
> 
> We have prepared a new patch and completed internal testing on our
> side. Without this patch, issues occur when using ufs-bsg.

This patch looks better. Can you please send it out as a separate email?

-- 
Jens Axboe

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

Posted by 라종휘 1 month, 3 weeks ago

Hello,

This is Jonghwi from Samsung. :)
I am sending you a patch via new email as requested.


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 <jonghwi.rha@samsung.com>

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) {


BRs,
Jonghwi,

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

Posted by Jens Axboe 1 month, 3 weeks ago

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 <jonghwi.rha@samsung.com>
> 
> 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?

-- 
Jens Axboe

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

Posted by Hannes Reinecke 1 month, 3 weeks ago

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 <jonghwi.rha@samsung.com>
>>
>> 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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

Posted by 라종휘 1 month, 3 weeks ago

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 <jonghwi.rha@samsung.com>
>>>
>>> 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 <jonghwi.rha@samsung.com>
---
 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,