Message-ID: <CAO9qdTH=tk4dSjULYS-yR+YOri8=55af8L+M6FRB-hsPRz6qww@mail.gmail.com>
Date: Thu, 24 Apr 2025 15:36:46 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Liam.Howlett@...cle.com, akpm@...ux-foundation.org, willy@...radead.org, 
	linux-kernel@...r.kernel.org, 
	syzbot+a2b84e569d06ca3a949c@...kaller.appspotmail.com
Subject: Re: [PATCH v2] ipc: fix to protect IPCS lookups using RCU

Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
>
> You sent this mail twice by mistake, and didn't link to the original
> thread, since you sent v1 2 months ago it's really useful to provide a link
> (here it's [0] ).
>
> [0]: https://lore.kernel.org/all/20250214180157.10288-1-aha310510@gmail.com/
>
> Your description is entirely missing any 'a syzkaller report encountered an
> issue whereby [blah blah blah]'. Please add this.
>
> On Tue, Apr 22, 2025 at 09:48:43PM +0900, Jeongjun Park wrote:
> > idr_for_each() is protected by rwsem, but this is not enough. If it is not
> > protected by the RCU read-critical region, when we call radix_tree_node_free()
> > via call_rcu() to free the struct radix_tree_node, the node will be freed
> > immediately, and when we read the next node in radix_tree_for_each_slot(),
> > we can read the already freed memory.
>
> What is calling call_rcu(), or radix_tree_for_each_slot() etc.? Presumably
> idr_for_each? Maybe worth saying soe xplicitly.
>
> >
> > Therefore, we need to add code to make sure that idr_for_each() is protected
> > within the RCU read-critical region when we call it in shm_destroy_orphaned().
> >
> > Reported-by: syzbot+a2b84e569d06ca3a949c@...kaller.appspotmail.com
>
> In Matthew's reply to you in the original thread he says 'if anybody is
> wondering what the hell this is about [link]'. It would have been useful to
> include this link here [1].
>
> [1]: https://lore.kernel.org/linux-kernel/67af13f8.050a0220.21dd3.0038.GAE@google.com/
>
> > Fixes: b34a6b1da371 ("ipc: introduce shm_rmid_forced sysctl")
> > Cc: Matthew Wilcox <willy@...radead.org>
>
> You're cc-ing more people than this? There's no need for you to manually
> add Cc: lines anyway. But this is just incorrect.
>
> > Signed-off-by: Jeongjun Park <aha310510@...il.com>
>
> Matthew seemed more aware of the behaviour in this code so I'm going to
> keep my review to process nits mostly.
>
> Others can actually review the change... But it seems you are doing what
> Matthew suggested broadly.
>
> Have you run with lockdep (i.e. CONFIG_PROVE_LOCKING) enabled? What testing
> have you done?
>
> Presumably it's ok to order the rcu read critical section and rwsem locks
> this way?

To test if this patch is the correct one, I applied this patch and
CONFIG_PROVE_LOCKING and reproduced the backtrace in the syzbot report [0]
through test.c

Unfortunately, I think I need to supplement test.c more to reproduce the
KASAN crash log, but I was able to successfully call idr_for_each()
similer to the syzbot backtrace.

Fortunately, the test results showed that there were no particular
problems related to RCU, and considering that shm_destroy_orphaned() and
proc_ipc_dointvec_minmax_orphans() are only called through proc_handler,
it can be confirmed that there are no particular problems with the patch.

I'll send you the v3 patch after fixing the other things you advised.

Regards,

Jeongjun Park

[0]: https://syzkaller.appspot.com/bug?extid=a2b84e569d06ca3a949c

test.c:
```c
// gcc -o test test.c

#define _GNU_SOURCE

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sched.h>
#include <fcntl.h>
#include <err.h>
#include <ctype.h>
#include <errno.h>
#include <time.h>
#include <inttypes.h>
#include <math.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <sys/types.h>
#include <sys/syscall.h>

#define KEY_ID 43214321
#define SHM_SIZE 4096
#define SHM_COUNT 100

#define FAIL_IF(x) if ((x)) { \
    perror(#x); \
    return -1; \
}

void write_file(const char *filename, char *text) {

    int fd = open(filename, O_RDWR);

    write(fd, text, strlen(text));
    close(fd);
}

void setup_ns(void) {

    uid_t uid = getuid();
    gid_t gid = getgid();
    char buffer[0x100];

    if (unshare(CLONE_NEWUSER | CLONE_NEWNS | CLONE_NEWIPC)) {
        perror(" [-] unshare(CLONE_NEWUSER | CLONE_NEWNS | CLONE_NEWIPC)");
exit(EXIT_FAILURE);
}

    write_file("/proc/self/setgroups", "deny");
    snprintf(buffer, sizeof(buffer), "0 %d 1", uid);
    write_file("/proc/self/uid_map", buffer);
    snprintf(buffer, sizeof(buffer), "0 %d 1", gid);
    write_file("/proc/self/gid_map", buffer);
}

int main() {
    setup_ns();

    int key_id, shm_id;

    for (int i = 0; i < SHM_COUNT; i++) {
        key_id = i + KEY_ID;
        shm_id = shmget((key_t)key_id, SHM_SIZE, IPC_CREAT | 0666);
        if (shm_id == -1)
            FAIL_IF("main: shmget error\n");
        void *shmaddr = shmat(shm_id, (void*)0, 0);
        if (shmaddr == -1)
            FAIL_IF("main: shmat error\n");

        int fd = open("/proc/sys/kernel/shm_rmid_forced", O_RDWR);
        if (fd <  0)
            FAIL_IF("open /proc/sys/kernel/shm_rmid_forced error\n");

        write(fd, "1", 1);
        close(fd);
    }

    return 0;
}
```

>
> > ---
> > v2: Change description and coding style
>
> Actually you completely change what you're doing here, this is a completely
> inaccurate changelog, before you dropped the rwsem, now you don't!! You
> should note this (i.e. - what it is you're actually doing...)
>
> Not sure what on earth you are changing in the coding style? You're adding
> braces and 2 lines of code?
>
> I don't think you really need to note a change in description, that's
> implicit.
>
> Also a:
>
> v1:
> https://lore.kernel.org/all/20250214180157.10288-1-aha310510@gmail.com/
>
> Is vital esp. after such a delay, as noted above.
>
> > ---
> >  ipc/shm.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 99564c870084..492fcc699985 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -431,8 +431,11 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
> >  void shm_destroy_orphaned(struct ipc_namespace *ns)
> >  {
> >       down_write(&shm_ids(ns).rwsem);
> > -     if (shm_ids(ns).in_use)
> > +     if (shm_ids(ns).in_use) {
> > +             rcu_read_lock();
> >               idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
> > +             rcu_read_unlock();
> > +     }
> >       up_write(&shm_ids(ns).rwsem);
> >  }
> >
> > --

Powered by blists - more mailing lists