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

  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() {
    struct request request;
    DECLARE_COMPLETION(done)
    request->done = done;
    padata = &request.padata;
    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 <wangjinchao@xfusion.com>
Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>
---
V3: 
    Include Daniel's ack
V2: https://lore.kernel.org/all/ZRTLHY5A+VqIKhA2@fedora/
    To satisfy Sparse, use rcu_dereference_protected.
    Reported-by: kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@intel.com/

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

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

Wang Jinchao <wangjinchao@xfusion.com> wrote:
>
> diff --git a/kernel/padata.c b/kernel/padata.c
> index 222d60195de6..79d04a97ded6 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -1102,12 +1102,16 @@ EXPORT_SYMBOL(padata_alloc_shell);
>  */
> void padata_free_shell(struct padata_shell *ps)
> {
> +       struct parallel_data *pd;
> +
>        if (!ps)
>                return;
> 
>        mutex_lock(&ps->pinst->lock);
>        list_del(&ps->list);
> -       padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> +       pd = rcu_dereference_protected(ps->pd, 1);
> +       if (refcount_dec_and_test(&pd->refcnt))
> +               padata_free_pd(rcu_dereference_protected(ps->pd, 1));

Why is this dereferencing ps->pd again after the refcount_dec_and_test?

If this is necessary please explain it in the code because it is not
at all obvious.

Thanks,
  

Hi,

On Sat, Oct 07, 2023 at 11:52:33AM +0800, Wang Jinchao wrote:
> Signed-off-by: Wang Jinchao <wangjinchao@xfusion.com>

The Fixes: tag should be near your Signed-off-by:, and the subject of
the patch should be something descriptive like

    padata: Fix refcnt handling in padata_free_shell()

Here's some documentation about this:

    https://docs.kernel.org/process/5.Posting.html#patch-formatting-and-changelogs

> Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> ---
> V3: 
>     Include Daniel's ack
>
> V2: https://lore.kernel.org/all/ZRTLHY5A+VqIKhA2@fedora/
>     To satisfy Sparse, use rcu_dereference_protected.
>     Reported-by: kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@intel.com/

These two tags can also go near your SoB.

> V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/
> 
>  kernel/padata.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/padata.c b/kernel/padata.c
> index 222d60195de6..79d04a97ded6 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -1102,12 +1102,16 @@ EXPORT_SYMBOL(padata_alloc_shell);
>   */
>  void padata_free_shell(struct padata_shell *ps)
>  {
> +	struct parallel_data *pd;
> +
>  	if (!ps)
>  		return;
>  
>  	mutex_lock(&ps->pinst->lock);
>  	list_del(&ps->list);
> -	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> +	pd = rcu_dereference_protected(ps->pd, 1);
> +	if (refcount_dec_and_test(&pd->refcnt))
> +		padata_free_pd(rcu_dereference_protected(ps->pd, 1));

As Herbert points out, this version changes the code by
rcu-dereferencing pd twice.  Usually previous acks are dropped when
introducing new changes, but you can have it back by only
rcu-dereferencing once and addressing the other comments above.

>  	mutex_unlock(&ps->pinst->lock);
>  
>  	kfree(ps);
> -- 
> 2.40.0
>
  

On Fri, Oct 13, 2023 at 11:22:03AM -0400, Daniel Jordan wrote:
> Hi,
> 
> On Sat, Oct 07, 2023 at 11:52:33AM +0800, Wang Jinchao wrote:
> > Signed-off-by: Wang Jinchao <wangjinchao@xfusion.com>
> 
> The Fixes: tag should be near your Signed-off-by:, and the subject of
> the patch should be something descriptive like
> 
>     padata: Fix refcnt handling in padata_free_shell()
> 
> Here's some documentation about this:
> 
>     https://docs.kernel.org/process/5.Posting.html#patch-formatting-and-changelogs
> 
Updated in patch v4.

> > Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> > ---
> > V3: 
> >     Include Daniel's ack
> >
> > V2: https://lore.kernel.org/all/ZRTLHY5A+VqIKhA2@fedora/
> >     To satisfy Sparse, use rcu_dereference_protected.
> >     Reported-by: kernel test robot <lkp@intel.com>
> >     Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@intel.com/
> 
> These two tags can also go near your SoB.
> 
From intel kernel test robot:
    If you fix the issue in a separate patch/commit (i.e. not just a new version of
    the same patch/commit), kindly add following tags
    | Reported-by: kernel test robot <lkp@intel.com>
    | Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@intel.com/
So I updated the patch without these tags.

> > V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/
> > 
> >  kernel/padata.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index 222d60195de6..79d04a97ded6 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -1102,12 +1102,16 @@ EXPORT_SYMBOL(padata_alloc_shell);
> >   */
> >  void padata_free_shell(struct padata_shell *ps)
> >  {
> > +	struct parallel_data *pd;
> > +
> >  	if (!ps)
> >  		return;
> >  
> >  	mutex_lock(&ps->pinst->lock);
> >  	list_del(&ps->list);
> > -	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> > +	pd = rcu_dereference_protected(ps->pd, 1);
> > +	if (refcount_dec_and_test(&pd->refcnt))
> > +		padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> 
> As Herbert points out, this version changes the code by
> rcu-dereferencing pd twice.  Usually previous acks are dropped when
> introducing new changes, but you can have it back by only
> rcu-dereferencing once and addressing the other comments above.
Thanks for your and Herbert's acks, which were included in patch v4.

I know Herbert has acked my patch from your email; however, I have 
not received Herbert's acknowledgment email, and I do not know why.
> 
> >  	mutex_unlock(&ps->pinst->lock);
> >  
> >  	kfree(ps);
> > -- 
> > 2.40.0
> >