linux-kernel - Re: [PATCH 1/1] sched/fair: Fix invalid pointer dereference in child_cfs_rq_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bb9f62885d138658c853d9c915f15438ba3755a.camel@linux.ibm.com>
Date: Wed, 05 Mar 2025 15:48:25 +0530
From: Aboorva Devarajan <aboorvad@...ux.ibm.com>
To: Vincent Guittot <vincent.guittot@...aro.org>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        riel@...riel.com, dietmar.eggemann@....com, rostedt@...dmis.org,
        bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com, odin@...d.al,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] sched/fair: Fix invalid pointer dereference in
 child_cfs_rq_on_list()

On Wed, 2025-03-05 at 09:21 +0100, Vincent Guittot wrote:
> On Tue, 4 Mar 2025 at 18:00, Aboorva Devarajan <aboorvad@...ux.ibm.com> wrote:
> > 
> > In child_cfs_rq_on_list(), leaf_cfs_rq_list.prev is expected to point to
> > a valid cfs_rq->leaf_cfs_rq_list in the hierarchy. However, when accessed
> > from the first node in a list, leaf_cfs_rq_list.prev can incorrectly point
> > back to the list head (rq->leaf_cfs_rq_list) instead of another
> > cfs_rq->leaf_cfs_rq_list.
> > 
> > The function does not handle this case, leading to incorrect pointer
> > calculations and unintended memory accesses, which can result in a kernel
> > crash.
> > 
> > A recent attempt to reorder fields in struct rq exposed this issue by
> > modifying memory offsets and affecting how pointer computations are
> > resolved. While the problem existed before, it was previously masked by
> > specific field arrangement. The reordering caused erroneous pointer
> > accesses, leading to a NULL dereference and a crash, as seen in the
> > following trace:
> > 
> > [    2.152852] Call Trace:
> > [    2.152855] __update_blocked_fair+0x45c/0x6a0 (unreliable)
> > [    2.152862] sched_balance_update_blocked_averages+0x11c/0x24c
> > [    2.152869] sched_balance_softirq+0x60/0x9c
> > [    2.152876] handle_softirqs+0x148/0x3b4
> > [    2.152884] do_softirq_own_stack+0x40/0x54
> > [    2.152891] __irq_exit_rcu+0x18c/0x1b4
> > [    2.152897] irq_exit+0x20/0x38
> > [    2.152903] timer_interrupt+0x174/0x30c
> > [    2.152910] decrementer_common_virt+0x28c/0x290
> > [    2.059873] systemd[1]: Hostname set to ...
> > [    2.152682] BUG: Unable to handle kernel data access on read at 0x100000125
> > [    2.152717] Faulting instruction address: 0xc0000000001c0270
> > [    2.152724] Oops: Kernel access of bad area, sig: 7 [#1]
> > ..
> > 
> > To fix this, introduce a check to detect when prev points to the list head
> > (&rq->leaf_cfs_rq_list). If this condition is met, return early to prevent
> > the use of an invalid prev_cfs_rq.
> > 
> > Fixes: fdaba61ef8a2 ("sched/fair: Ensure that the CFS parent is added after unthrottling")
> > Signed-off-by: Aboorva Devarajan <aboorvad@...ux.ibm.com>
> > ---
> >  kernel/sched/fair.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 1c0ef435a7aa..a4daa7a9af0b 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4045,12 +4045,15 @@ static inline bool child_cfs_rq_on_list(struct cfs_rq *cfs_rq)
> >  {
> >         struct cfs_rq *prev_cfs_rq;
> >         struct list_head *prev;
> > +       struct rq *rq;
> > +
> > +       rq = rq_of(cfs_rq);
> > 
> >         if (cfs_rq->on_list) {
> >                 prev = cfs_rq->leaf_cfs_rq_list.prev;
> > +               if (prev == &rq->leaf_cfs_rq_list)
> > +                       return false;
> 
> what about the else case below , prev can also point to rq->leaf_cfs_rq_list

Hi Vincent,

Thanks for the comments, yes indeed `rq->tmp_alone_branch` canpoint to `rq->leaf_cfs_rq_list`

I overlooked this and assumed that as long as we have at least one cfs_rq, tmp_alone_branch
would always point to a valid list node (cfs_rq->leaf_cfs_rq_list).

I will send a v2 with the changes to carry out the correctness check unconditionally.


> >         } else {
> > -               struct rq *rq = rq_of(cfs_rq);
> > -
> >                 prev = rq->tmp_alone_branch;
> >         }
> > 
> > --
> > 2.43.5
> > 

- Aboorva