From: Sebastien Marie Subject: patch: relax ni_pledge panic To: tech@openbsd.org Date: Thu, 06 Feb 2025 10:23:48 +0100 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 (2025-02-06 08:55:58) Committer: Sebastien Marie (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