Re: [PATCH v2] eventfs: Use children field for rcu head and add memory barriers

[Steven Rostedt]

[ Replying to Sashiko ]

> --- Patch [1]: [PATCH v2] eventfs: Use children field for rcu head and add 
> memory barriers ---
> commit 4a672c87272cb1dcd1b1464842254420c8ac2bec
> Author: Steven Rostedt <rost...@goodmis.org>
> 
> eventfs: Use children field for rcu head and add memory barriers
> 
> When an eventfs inode is freed, it sets ei->is_freed and adds it to the
> srcu link list. This patch changes the union of the rcu list head with
> the children list and adds memory barriers to prevent SRCU iteration
> from reading a corrupt target.
> 
> Closes: 
> https://sashiko.dev/#/patchset/20260806022719.375354-1-shuangpeng.kernel%40gmail.com
> 
> This is a pre-existing issue, but does eventfs_iterate() in
> fs/tracefs/event_inode.c truncate ctx->pos in a way that bypasses loop
> bounds checks?
> 
> If a user uses lseek(fd, offset, SEEK_SET) to set the 64-bit loff_t
> ctx->pos to 0x100000000 (4 GiB):

This shouldn't be a problem as tracefs/eventfs uses the default
maxbytes which is defined as:

  fs/super.c:     s->s_maxbytes = MAX_NON_LFS;
  include/linux/fs.h:#define      MAX_NON_LFS     ((1UL<<31) - 1)

Which ends up being 2147483647 (0x7fffffff). I tested this using code:

        unsigned long long addr;
        struct dirent *dirent;
        DIR *dir;
        char *file;
        int fd;

        if (argc != 3) {
                fprintf(stderr,"usage: %s dir addr\n",argv[0]);
                exit(-1);
        }

        file = argv[1];
        addr = strtoull(argv[2], NULL, 0);

        fd = open(file, O_RDONLY);
        if (fd < 0) {
                perror("open");
                exit(0);
        }
        printf("lseek to %llx\n", addr);
        if (lseek64(fd, addr, SEEK_SET) == (off_t)-1) {
                perror("lseek");
                exit(-1);
        }
        dir = fdopendir(fd);
        dirent = readdir(dir);
        if (!dirent) {
                printf("Nothing to read\n");
                exit(0);
        }
        printf("entry: %s\n", dirent->d_name);

The program was called "readdir" and I ran it like this:

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 2147483647
lseek to 7fffffff
Nothing to read

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 2147483648
lseek to 80000000
lseek: Invalid argument

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 5
lseek to 5
entry: filter

Which proves that pos will never be 0x100000000

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 0x100000000
lseek to 100000000
lseek: Invalid argument

-- Steve




> 
> fs/tracefs/event_inode.c:eventfs_iterate() {
>     ...
>     c = ctx->pos - 2;
> 
> Here, c = ctx->pos - 2 computes 0xFFFFFFFE, which is truncated into the
> 32-bit signed integer c as -2. The loop variable i then starts at -2:
> 
>     for (i = c; i < ei->nr_entries; i++, ctx->pos++) {
>         void *cdata = ei->data;
> 
>         entry = &ei->entries[i];
> 
> Per C integer promotion rules, the 30-bit unsigned bitfield ei->nr_entries
> is promoted to a signed int. The check evaluates as -2 < (int)ei->nr_entries,
> bypassing the bounds check. This allows an out-of-bounds array read on
> &ei->entries[-2], and subsequently calls entry->callback.
>