Re: [PATCH 5/5] padata: avoid potential UAFs to the padata_shell from padata_reorder()

[Daniel Jordan]

On Wed, Nov 09, 2022 at 02:03:29PM +0100, Nicolai Stange wrote:
> Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:
> 
> > On Wed, Oct 19, 2022 at 10:37:08AM +0200, Nicolai Stange wrote:
> >> Even though the parallel_data "pd" instance passed to padata_reorder() is
> >> guaranteed to exist as per the reference held by its callers, the same is
> >> not true for the associated padata_shell, pd->ps. More specifically, once
> >> the last padata_priv request has been completed, either at entry from
> >> padata_reorder() or concurrently to it, the padata API users are well
> >> within their right to free the padata_shell instance.
> >
> > The synchronize_rcu change seems to make padata_reorder safe from freed
> > ps's with the exception of a straggler reorder_work.  For that, I think
> > something like this hybrid of your code and mine is enough to plug the
> > hole.  It's on top of 1-2 and my hunk from 3.  It has to take an extra
> > ref on pd, but only in the rare case where the reorder work is used.
> > Thoughts?
> >
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index cd6740ae6629..f14c256a0ee3 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -277,7 +277,7 @@ static struct padata_priv *padata_find_next(struct parallel_data *pd,
> >  
> >  static void padata_reorder(struct parallel_data *pd)
> >  {
> > -	struct padata_instance *pinst = pd->ps->pinst;
> > +	struct padata_instance *pinst;
> >  	int cb_cpu;
> >  	struct padata_priv *padata;
> >  	struct padata_serial_queue *squeue;
> > @@ -314,7 +314,7 @@ static void padata_reorder(struct parallel_data *pd)
> >  		list_add_tail(&padata->list, &squeue->serial.list);
> >  		spin_unlock(&squeue->serial.lock);
> >  
> > -		queue_work_on(cb_cpu, pinst->serial_wq, &squeue->work);
> > +		queue_work_on(cb_cpu, pd->ps->pinst->serial_wq, &squeue->work);
> >  	}
> >  
> >  	spin_unlock_bh(&pd->lock);
> > @@ -330,8 +330,10 @@ static void padata_reorder(struct parallel_data *pd)
> >  	smp_mb();
> >  
> >  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
> > -	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
> > -		queue_work(pinst->serial_wq, &pd->reorder_work);
> > +	if (!list_empty(&reorder->list) && padata_find_next(pd, false)) {
> > +		if (queue_work(pd->ps->pinst->serial_wq, &pd->reorder_work))
> > +			padata_get_pd(pd);
> 
> As the reorder_work can start running immediately after having been
> submitted, wouldn't it be more correct to do something like
> 
> 		padata_get_pd(pd);
> 		if (!queue_work(pd->ps->pinst->serial_wq, &pd->reorder_work))
> 			padata_put_pd(pd);
> 
> ?

Yes, that's better, and all the above can go in your next version too.