padata: Fix the UAF issue related to parallel_data

PATCH In review Wang Jinchao 2 participants 4 messages Sep 28, 2023 – Oct 7, 2023

Subsystems (2)

Crypto API Padata parallel execution mechanism

lore

Share & tools

lore

Wang Jinchao

author

2023-09-28 00:39 UTC #0 lore raw

PATCH v2 1/1 padata: Fix the UAF issue related to parallel_data

In a high-load arm64 environment, the pcrypt_aead01 test in LTP can lead to system UAF (Use-After-Free) issues. Due to the lengthy analysis of the pcrypt_aead01 function call, I'll describe the problem scenario using a simplified model:

Suppose there's a user of padata named user_function that adheres to the padata requirement of calling padata_free_shell after serial() has been invoked, as demonstrated in the following code:

```c
struct request {
    struct padata_priv padata;
    struct completion *done;
};

void parallel(struct padata_priv *padata) {
    do_something();
}

void serial(struct padata_priv *padata) {
    struct request *request = container_of(padata, struct request, padata);
    complete(request->done);
}

void user_function() {
    DECLARE_COMPLETION(done)
    padata->parallel = parallel;
    padata->serial = serial;
    padata_do_parallel();
    wait_for_completion(&done);
    padata_free_shell();
}
```

In the corresponding padata.c file, there's the following code:

```c
static void padata_serial_worker(struct work_struct *serial_work) {
    ...
    cnt = 0;

    while (!list_empty(&local_list)) {
        ...
        padata->serial(padata);
        cnt++;
    }

    local_bh_enable();

    if (refcount_sub_and_test(cnt, &pd->refcnt))
        padata_free_pd(pd);
}
```

Because of the high system load and the accumulation of unexecuted softirq at this moment, local_bh_enable() in padata takes longer to execute than usual. Subsequently, when accessing pd->refcnt, pd has already been released by padata_free_shell(), resulting in a UAF issue with pd->refcnt.

The fix is straightforward: add refcount_dec_and_test before calling padata_free_pd in padata_free_shell.

Signed-off-by Wang Jinchao <[email protected]>

---
V2:
To satisfy Sparse, use rcu_dereference_protected.

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/

 kernel/padata.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/padata.c +3 -1

index 222d60195de6..acef1e703a8b 100644
@@ -1107,7 +1107,9 @@ void padata_free_shell(struct padata_shell *ps)
 
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
-	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
+	struct parallel_data *pd = rcu_dereference_protected(ps->pd, 1);
+	if (refcount_dec_and_test(&pd->refcnt))
+		padata_free_pd(pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);

Signature · 1 line

2.40.0

Daniel Jordan

maintainer Acked-by on v2 1/1

2023-10-04 14:54 UTC #1 lore raw

Quoted 54 lines · Wang Jinchao

In a high-load arm64 environment, the pcrypt_aead01 test in LTP can lead to system UAF (Use-After-Free) issues. Due to the lengthy analysis of the pcrypt_aead01 function call, I'll describe the problem scenario using a simplified model:
Suppose there's a user of padata named user_function that adheres to the padata requirement of calling padata_free_shell after serial() has been invoked, as demonstrated in the following code:
```c
struct request {
    struct padata_priv padata;
    struct completion *done;
};

void parallel(struct padata_priv *padata) {
    do_something();
}

void serial(struct padata_priv *padata) {
    struct request *request = container_of(padata, struct request, padata);
    complete(request->done);
}

void user_function() {
    DECLARE_COMPLETION(done)
    padata->parallel = parallel;
    padata->serial = serial;
    padata_do_parallel();
    wait_for_completion(&done);
    padata_free_shell();
}
```
In the corresponding padata.c file, there's the following code:
```c
static void padata_serial_worker(struct work_struct *serial_work) {
    ...
    cnt = 0;

    while (!list_empty(&local_list)) {
        ...
        padata->serial(padata);
        cnt++;
    }

    local_bh_enable();

    if (refcount_sub_and_test(cnt, &pd->refcnt))
        padata_free_pd(pd);
}
```

Quoted 8 lines

Because of the high system load and the accumulation of unexecuted softirq at this moment, local_bh_enable() in padata takes longer to execute than usual. Subsequently, when accessing pd->refcnt, pd has already been released by padata_free_shell(), resulting in a UAF issue with pd->refcnt.
The fix is straightforward: add refcount_dec_and_test before calling padata_free_pd in padata_free_shell.

This could use a Fixes tag. From Nicolai's patch[0] we agreed on

Fixes 07928d9bfc81 ("padata: Remove broken queue flushing")

With that,

Acked-by Daniel Jordan <[email protected]>

Thanks!

[0] https://lore.kernel.org/all/[email protected]/

Quoted 30 lines

Signed-off-by Wang Jinchao <[email protected]>
---
V2:
To satisfy Sparse, use rcu_dereference_protected.
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/
 kernel/padata.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/padata.c +3 -1
index 222d60195de6..acef1e703a8b 100644
@@ -1107,7 +1107,9 @@ void padata_free_shell(struct padata_shell *ps)
 
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
-	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
+	struct parallel_data *pd = rcu_dereference_protected(ps->pd, 1);
+	if (refcount_dec_and_test(&pd->refcnt))
+		padata_free_pd(pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);
Signature · 1 line
2.40.0

Daniel Jordan

maintainer

2023-10-04 19:09 UTC #2 lore raw

Quoted 94 lines · Daniel Jordan

Quoted 54 lines · Wang Jinchao
In a high-load arm64 environment, the pcrypt_aead01 test in LTP can lead to system UAF (Use-After-Free) issues. Due to the lengthy analysis of the pcrypt_aead01 function call, I'll describe the problem scenario using a simplified model:
Suppose there's a user of padata named user_function that adheres to the padata requirement of calling padata_free_shell after serial() has been invoked, as demonstrated in the following code:
```c
struct request {
    struct padata_priv padata;
    struct completion *done;
};

void parallel(struct padata_priv *padata) {
    do_something();
}

void serial(struct padata_priv *padata) {
    struct request *request = container_of(padata, struct request, padata);
    complete(request->done);
}

void user_function() {
    DECLARE_COMPLETION(done)
    padata->parallel = parallel;
    padata->serial = serial;
    padata_do_parallel();
    wait_for_completion(&done);
    padata_free_shell();
}
```
In the corresponding padata.c file, there's the following code:
```c
static void padata_serial_worker(struct work_struct *serial_work) {
    ...
    cnt = 0;

    while (!list_empty(&local_list)) {
        ...
        padata->serial(padata);
        cnt++;
    }

    local_bh_enable();

    if (refcount_sub_and_test(cnt, &pd->refcnt))
        padata_free_pd(pd);
}
```
Quoted 8 lines
Because of the high system load and the accumulation of unexecuted softirq at this moment, local_bh_enable() in padata takes longer to execute than usual. Subsequently, when accessing pd->refcnt, pd has already been released by padata_free_shell(), resulting in a UAF issue with pd->refcnt.
The fix is straightforward: add refcount_dec_and_test before calling padata_free_pd in padata_free_shell.
This could use a Fixes tag. From Nicolai's patch[0] we agreed on
Fixes 07928d9bfc81 ("padata: Remove broken queue flushing")
With that,
Acked-by Daniel Jordan <[email protected]>
Thanks!
[0] https://lore.kernel.org/all/[email protected]/
Quoted 18 lines
Signed-off-by Wang Jinchao <[email protected]>
---
V2:
To satisfy Sparse, use rcu_dereference_protected.
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/
 kernel/padata.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/padata.c +0 -0
index 222d60195de6..acef1e703a8b 100644
@@ -1107,7 +1107,9 @@ void padata_free_shell(struct padata_shell *ps)
 

Quoted 4 lines

kernel/padata.c void padata_free_shell(struct padata_shell *ps) · line 1110
Quoted 4 lines
kernel/padata.c +1 -1
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
-	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
+	struct parallel_data *pd = rcu_dereference_protected(ps->pd, 1);

Oh, I should've also said please move the declaration of pd to the top of the function like scripts/checkpatch.pl suggests.

Quoted 8 lines

kernel/padata.c +2 -0

+	if (refcount_dec_and_test(&pd->refcnt))
+		padata_free_pd(pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);

Signature · 1 line

2.40.0

Wang Jinchao

author

2023-10-07 04:02 UTC #3 lore raw

Quoted 2 lines · Daniel Jordan

Quoted 1 line · Daniel Jordan
On Thu, Sep 28, 2023 at 08:38:53AM +0800, Wang Jinchao wrote:

...

Quoted 10 lines

Quoted 10 lines
This could use a Fixes tag. From Nicolai's patch[0] we agreed on
Fixes 07928d9bfc81 ("padata: Remove broken queue flushing")
With that,
Acked-by Daniel Jordan <[email protected]>
Thanks!

...

Quoted 4 lines

Oh, I should've also said please move the declaration of pd to the top of the function like scripts/checkpatch.pl suggests.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
kernel/padata.c +2 -0
+	if (refcount_dec_and_test(&pd->refcnt))
+		padata_free_pd(pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);
Signature · 1 line
2.40.0

Thanks for suggestion, both were updated in V3.
V3: https://lore.kernel.org/all/ZSDWAcUxXcwD4YUZ@fedora/