psi: Fix uaf issue when psi trigger is destroyed while being polled

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>

---

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>

---

Thanks, I'll go stick this in sched/urgent unless Linus picks it up himself.

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>

Quoted 11 lines · Suren Baghdasaryan

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Quoted 6 lines

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Quoted 1 line

kernel/sched/psi.c:1182:6: warning: no previous prototype for function 'psi_trigger_destroy' [-Wmissing-prototypes]

)

Quoted 13 lines · Johannes Weiner

Quoted 11 lines · Suren Baghdasaryan

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Quoted 8 lines

Quoted 6 lines

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Hmm. kernel test robot notified me of new (which are not really new) warnings but I don't think this patch specifically introduced them:

kernel/sched/psi.c:1112:21: warning: no previous prototype for function 'psi_trigger_create' [-Wmissing-prototypes]
struct psi_trigger *psi_trigger_create(struct psi_group *group,

                       ^

kernel/sched/psi.c:1112:1: note: declare 'static' if the function
is not intended to be used outside of this translation unit
struct psi_trigger *psi_trigger_create(struct psi_group *group, ^
static

Quoted 1 line

Quoted 1 line

kernel/sched/psi.c:1182:6: warning: no previous prototype for function 'psi_trigger_destroy' [-Wmissing-prototypes]

void psi_trigger_destroy(struct psi_trigger *t)

        ^

kernel/sched/psi.c:1182:1: note: declare 'static' if the function
is not intended to be used outside of this translation unit
void psi_trigger_destroy(struct psi_trigger *t)
^
static
kernel/sched/psi.c:1249:10: warning: no previous prototype for
function 'psi_trigger_poll' [-Wmissing-prototypes]
__poll_t psi_trigger_poll(void **trigger_ptr,

            ^

kernel/sched/psi.c:1249:1: note: declare 'static' if the function
is not intended to be used outside of this translation unit
__poll_t psi_trigger_poll(void **trigger_ptr,
^

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

struct psi_trigger *_psi_trigger_create(struct psi_group *group, char *buf, size_t nbytes, enum psi_res res)
{
// original psi_trigger_create code
}

#ifdef CONFIG_CGROUPS

struct psi_trigger *psi_trigger_create(struct psi_group *group, char *buf, size_t nbytes, enum psi_res res)

{
    return _psi_trigger_create(group, buf, nbytes, res);
}

#else

static struct psi_trigger *psi_trigger_create(struct psi_group *group, char *buf, size_t nbytes, enum psi_res res)

{
    return _psi_trigger_create(group, buf, nbytes, res);
}

#endif

Two questions:
1. Is this even worth fixing?
2. If so, I would like to do that as a separate patch (these warnings are unrelated to the changes in this patch). Would that be ok? Thanks,
Suren.

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Quoted 37 lines · Matthew Wilcox

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Quoted 7 lines

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Thanks Matthew!
That looks much cleaner. I'll post a separate patch to fix these. My main concern was whether it's worth adding more code to satisfy this warning but with this approach the code changes are minimal, so I'll go ahead and post it shortly.

Quoted 46 lines · Suren Baghdasaryan

Quoted 37 lines · Matthew Wilcox

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Quoted 7 lines

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Quoted 5 lines

Thanks Matthew!
That looks much cleaner. I'll post a separate patch to fix these. My main concern was whether it's worth adding more code to satisfy this warning but with this approach the code changes are minimal, so I'll go ahead and post it shortly.

Why not simply move the declarations of psi_trigger_create() and psi_trigger_destroy() in include/linux/psi.h outside of the '#ifdef CONFIG_CGROUPS' block, to match the .c file?

They *could* be static when !CONFIG_CGROUPS, but IMO it's not worth bothering.

- Eric

Quoted 54 lines · Eric Biggers

Quoted 51 lines · Suren Baghdasaryan

Quoted 37 lines · Matthew Wilcox

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Quoted 7 lines

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Thanks Matthew!
That looks much cleaner. I'll post a separate patch to fix these. My main concern was whether it's worth adding more code to satisfy this warning but with this approach the code changes are minimal, so I'll go ahead and post it shortly.

Quoted 3 lines

Why not simply move the declarations of psi_trigger_create() and psi_trigger_destroy() in include/linux/psi.h outside of the '#ifdef CONFIG_CGROUPS' block, to match the .c file?

IIRC this was done to avoid another warning that these functions are not used outside of psi.c when CONFIG_CGROUPS=n

Quoted 59 lines · Suren Baghdasaryan

Quoted 57 lines · Eric Biggers

Quoted 46 lines · Suren Baghdasaryan

Quoted 37 lines · Matthew Wilcox

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Quoted 7 lines

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Quoted 5 lines

Thanks Matthew!
That looks much cleaner. I'll post a separate patch to fix these. My main concern was whether it's worth adding more code to satisfy this warning but with this approach the code changes are minimal, so I'll go ahead and post it shortly.

Why not simply move the declarations of psi_trigger_create() and psi_trigger_destroy() in include/linux/psi.h outside of the '#ifdef CONFIG_CGROUPS' block, to match the .c file?

Quoted 3 lines

IIRC this was done to avoid another warning that these functions are not used outside of psi.c when CONFIG_CGROUPS=n

What tool gave that warning?

- Eric

Quoted 61 lines · Eric Biggers

Quoted 59 lines · Suren Baghdasaryan

Quoted 57 lines · Eric Biggers

Quoted 46 lines · Suren Baghdasaryan

Quoted 37 lines · Matthew Wilcox

Quoted 10 lines · Suren Baghdasaryan

Quoted 10 lines

This happens with the following config:

CONFIG_CGROUPS=n
CONFIG_PSI=y

With cgroups disabled these functions are defined as non-static but are not defined in the header
(https://elixir.bootlin.com/linux/latest/source/include/linux/psi.h#L28)
since the only external user cgroup.c is disabled. The cleanest way to fix these I think is by doing smth like this in psi.c:

A cleaner way to solve these is simply:

#ifndef CONFIG_CGROUPS
static struct psi_trigger *psi_trigger_create(...);
...
#endif

I tested this works:

$ cat foo5.c
static int psi(void *);

int psi(void *x)

{
        return (int)(long)x;
}

int bar(void *x)

{
        return psi(x);
}
$ gcc -W -Wall -O2 -c -o foo5.o foo5.c
$ readelf -s foo5.o

Quoted 7 lines

Symbol table '.symtab' contains 4 entries:

   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS foo5.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     3 FUNC    GLOBAL DEFAULT    1 bar

Quoted 5 lines

Thanks Matthew!
That looks much cleaner. I'll post a separate patch to fix these. My main concern was whether it's worth adding more code to satisfy this warning but with this approach the code changes are minimal, so I'll go ahead and post it shortly.

Why not simply move the declarations of psi_trigger_create() and psi_trigger_destroy() in include/linux/psi.h outside of the '#ifdef CONFIG_CGROUPS' block, to match the .c file?

Quoted 5 lines

Quoted 3 lines

IIRC this was done to avoid another warning that these functions are not used outside of psi.c when CONFIG_CGROUPS=n

What tool gave that warning?

Let me double-check by building it. It has been a while since I developed the code and I don't want to mislead by making false claims.

Quoted 2 lines

- Eric