[PATCH][RFC] dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

[PATCH][RFC] dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

[Li RongQing]

I think when the description is freed, it maybe used again because of
race, then it's next maybe pointer a value that should not be freed

To prevent this, list_for_each_entry_safe should be used.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/dma/idxd/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 8dc029c..0c7fed7 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -611,7 +611,7 @@ static void irq_process_work_list(struct idxd_irq_entry *irq_entry)
 
 	spin_unlock(&irq_entry->list_lock);
 
-	list_for_each_entry(desc, &flist, list) {
+	list_for_each_entry_safe(desc, n, &flist, list) {
 		/*
 		 * Check against the original status as ABORT is software defined
 		 * and 0xff, which DSA_COMP_STATUS_MASK can mask out.
-- 
2.9.4

RE: [PATCH][RFC] dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

[Li,Rongqing]

> I think when the description is freed, it maybe used again because of race, then
> it's next maybe pointer a value that should not be freed
> 
> To prevent this, list_for_each_entry_safe should be used.
> 

Rewrite the commit header:

    dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

    The description has been freed in idxd_desc_complete(), should not be used
    in iteration, so replace list_for_each_entry with list_for_each_entry_safe
    to prevent the accessing

    Signed-off-by: Li RongQing <lirongqing@baidu.com>


> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/dma/idxd/irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c index
> 8dc029c..0c7fed7 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -611,7 +611,7 @@ static void irq_process_work_list(struct idxd_irq_entry
> *irq_entry)
> 
>  	spin_unlock(&irq_entry->list_lock);
> 
> -	list_for_each_entry(desc, &flist, list) {
> +	list_for_each_entry_safe(desc, n, &flist, list) {
>  		/*
>  		 * Check against the original status as ABORT is software defined
>  		 * and 0xff, which DSA_COMP_STATUS_MASK can mask out.
> --
> 2.9.4

Re: [PATCH][RFC] dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

[Dave Jiang]

On 5/26/24 11:19 PM, Li RongQing wrote:
> I think when the description is freed, it maybe used again because of
s/description/descriptor/

> race, then it's next maybe pointer a value that should not be freed
> 
> To prevent this, list_for_each_entry_safe should be used.
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/dma/idxd/irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index 8dc029c..0c7fed7 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -611,7 +611,7 @@ static void irq_process_work_list(struct idxd_irq_entry *irq_entry)
>  
>  	spin_unlock(&irq_entry->list_lock);
>  
> -	list_for_each_entry(desc, &flist, list) {
> +	list_for_each_entry_safe(desc, n, &flist, list) {
>  		/*
>  		 * Check against the original status as ABORT is software defined
>  		 * and 0xff, which DSA_COMP_STATUS_MASK can mask out.

I see what you are pointing at. I think you will also need a
list_del(&desc->list);
immediately after the comments.

Because if the descriptor is freed and given out to a different thread while the code is still walking the list, the iterator may hit a bad pointer due to the freed descriptor pointing to something else.

Also, please include a Fixes tag for the fix. Thanks!

RE: [External Mail] Re: [PATCH][RFC] dmaengine: idxd: Fix possible Use-After-Free in irq_process_work_list

[Li,Rongqing]

> Because if the descriptor is freed and given out to a different thread while the
> code is still walking the list, the iterator may hit a bad pointer due to the freed
> descriptor pointing to something else.
> 
> Also, please include a Fixes tag for the fix. Thanks!


Ok, I will send V2, thanks

Br