Index | Thread | Search

From:
Sebastien Marie <semarie@kapouay.eu.org>
Subject:
patch: relax ni_pledge panic
To:
tech@openbsd.org
Date:
Thu, 06 Feb 2025 10:23:48 +0100

Download raw body.

Thread
Hi,

The following patch relax the panic("ni_pledge") check in namei().

It was initially done to ensure coverage of all NDINIT() calls reachable
while pledged (it is a programming error to call namei() without
ni_pledge if pledged).

But with a race, it is possible to a unreachable syscall while pledged
(like acct(2) or chroot(2)) to trigger the panic.

syzbot reported the following panics:
- acct(2) : https://syzkaller.appspot.com/bug?extid=a8229b7ddd62e3a50eea
- chroot(2) : https://syzkaller.appspot.com/bug?id=f0ed88b77fd8900402613b1ebf01166049465abe

From my reading of the backtrace, it could occurs in the following
scenario:

[1] acct("/tmp/file")
    - we are not pledged, so the call reach sys_acct().
      NDINIT() doesn't set ni_pledge (we are not pledged),
      and call namei().

    - while in namei(), reach a sleeping point (for example,
      pool_get(PR_WAITOK) at line 145):

kern/vfs_lookup.c
 117   │ int
 118   │ namei(struct nameidata *ndp)
 119   │ {
...
 140   │        /*
 141   │         * Get a buffer for the name to be translated, and copy the
 142   │         * name into the buffer.
 143   │         */
 144   │        if ((cnp->cn_flags & HASBUF) == 0)
 145   │                cnp->cn_pnbuf = pool_get(&namei_pool, PR_WAITOK);
...

[2] in another thread, pledge("stdio rpath wpath"), and returns.
    the process is now pledged.

[1] resume from the sleeping point and continue

kern/vfs_lookup.c
...
 200   │        if (ndp->ni_cnd.cn_flags & KERNELPATH) {
 201   │                ndp->ni_cnd.cn_flags |= BYPASSUNVEIL;
 202   │        } else {
 203   │                error = pledge_namei(p, ndp, cnp->cn_pnbuf);
 204   │                if (error)
 205   │                        goto fail;
 206   │        }

the pledge_namei() will trigger panic("ni_pledge"). it is unexpected
that a syscall disallowed-while-pledged reach this codepath when
pledged.


I propose to relax the panic() to a simple pledge_fail(). It will make
the system still running (not panic anymore), and kill the
process. After all, at pledge_namei() time we are pledged, so it should
not continue.

The following diff does it.

Commit ID: 5296b38dc15fe733903493e84b5c88cbd65e2efb
Change ID: rrmmmquowkmqwkyllnqswrlvzyznuvuk
Author   : Sebastien Marie <semarie@kapouay.eu.org> (2025-02-06 08:55:58)
Committer: Sebastien Marie <semarie@kapouay.eu.org> (2025-02-06 09:23:21)

    pledge: relax panic("ni_pledge") and just error out instead of panic()

diff --git a/sys/kern/kern_pledge.c b/sys/kern/kern_pledge.c
index 89281cf0a2..7463a70d0f 100644
--- a/sys/kern/kern_pledge.c
+++ b/sys/kern/kern_pledge.c
@@ -595,8 +595,16 @@
 		return (0);
 	pledge = READ_ONCE(p->p_p->ps_pledge);
 
+	/*
+	 * ni_pledge is unset. it could be a programming bug (missing entry)
+	 * or some race between pledge(2) and another syscall.
+	 *
+	 * in both case, don't continue: we are pledged and this namei() call
+	 * is unwanted while pledged. pledge_fail() will report the error using
+	 * uprintf(9).
+	 */
 	if (ni->ni_pledge == 0)
-		panic("pledge_namei: ni_pledge");
+		return (pledge_fail(p, EPERM, 0));
 
 	/*
 	 * We set the BYPASSUNVEIL flag to skip unveil checks


Comments or OK ?
-- 
Sebastien Marie