[PATCH 2/2] binfmt_misc: fix racy checks in bpf set_interp kfuncs

From: Chris Mason <[email protected]>

bpf_binprm_set_interp() tests path[0] != '/' on the buffer its load
program passes and then reads the same buffer again to copy it with
kmemdup_nul(). The buffer can be a BPF map value that another CPU
rewrites between the two reads. If byte 0 is overwritten in that
window, the kfunc stages a relative or empty interpreter path. The
staged path is not checked again, so open_exec() resolves a relative
path against the working directory of the task doing the exec.
bpf_binprm_set_interp_arg() has the same pattern for its "!len" test
and can stage an empty argument, which the interpreter then receives
as an empty argv entry.

The verifier checks the path and path__sz pair with BPF_READ |
BPF_WRITE, so a writable array map value is an accepted argument.
bpf(BPF_MAP_UPDATE_ELEM) on an array map copies the new value over the
old one in place and takes no lock. Both kfuncs are KF_SLEEPABLE and
allocate with GFP_KERNEL between the test and the copy, so the task
can sleep inside the window:

    load program                          bpf(BPF_MAP_UPDATE_ELEM)
    bpf_binprm_set_interp()
      strnlen(path, path__sz)
      path[0] != '/' is false
      kmemdup_nul(path, len, GFP_KERNEL)
        allocation may sleep
                                          array_map_update_elem()
                                            copy_map_value()
                                              rewrites byte 0
        copy reads path again
      bm_bpf_stage_selection()

The test in the load program's column proves what byte 0 held only at
the moment the test ran. The map update takes no lock, so it can store
to byte 0 right after. kmemdup_nul() then copies the rewritten bytes,
and bm_bpf_stage_selection() publishes them as bprm->bpf_interp.

The staged path is not checked again on its way to open_exec():

    load_misc_binary()
      entry_select_interpreter()     returns bprm->bpf_interp unchanged
      build_interp_argv()
        copy_string_kernel()         copies it as argv[0]
      bprm_change_interp()
        kstrdup()
      entry_open_interpreter()
        open_exec()                  unless a bound file is staged or
                                     the entry is an 'F' entry

None of these functions tests the first byte, and load_misc_binary()
hands the pointer to nothing else.

In bpf_binprm_set_interp_arg(), strnlen() finds a non-zero len, a NUL
is then stored to byte 0, and build_interp_argv() later copies the
empty bprm->bpf_interp_arg with copy_string_kernel().

The handler's own load program has to pass a writable map value, and
something has to store into it while the kfunc runs. The allocation can
sleep inside the window, and with a BPF_F_MMAPABLE array the store is a
plain user space write into the mapped value, so a loop can hit it
without a single bpf() call.

Check the private copy in both kfuncs, so that the string that gets
staged is the string that was checked. bpf_binprm_select_interp()
already looks its name up in a private copy for the same reason. The
remaining tests work on path__sz, arg__sz or the local len, and the
copy length is len, so the copy stays inside the extent the verifier
checked.

Results of bpf_binprm_set_interp() with the check on the copy:

- A NUL stored to byte 0 fails interp[0] != '/' and gets -EINVAL.
- For len == 0, kmemdup_nul() returns an empty string, so an empty path
  still gets -EINVAL.
- A NUL stored further into the string only shortens it to another
  absolute path, or another non-empty argument, that the program could
  have passed anyway.
- A path that both lacks the leading '/' and is PATH_MAX or longer now
  gets -ENAMETOOLONG instead of -EINVAL.
- A path that is empty or lacks the leading '/' is now rejected after
  the copy rather than before it, so such a call makes an allocation
  and returns -ENOMEM instead of -EINVAL if that allocation fails.

bpf_binprm_set_interp_arg() still rejects an empty argument before
allocating, so its results are unchanged apart from the raced case
fixed here.

Both new checks run before the previously staged string is freed or
replaced. A failing call frees only its own allocation and leaves the
earlier selection in place, as the -ENOMEM path already does.

Fixes: b4bfe2f6b011 ("binfmt_misc: add binfmt_misc_ops bpf struct_ops")
Signed-off-by: Chris Mason <[email protected]>
Signed-off-by: Christian Brauner (Amutable) <[email protected]>
---
 fs/binfmt_misc_bpf.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_misc_bpf.c b/fs/binfmt_misc_bpf.c
index ce1bc78e8511..a3e26e8a4027 100644
--- a/fs/binfmt_misc_bpf.c
+++ b/fs/binfmt_misc_bpf.c
@@ -141,8 +141,6 @@ __bpf_kfunc int bpf_binprm_set_interp(struct linux_binprm *bprm,
 	len = strnlen(path, path__sz);
 	if (len == path__sz)
 		return -EINVAL;
-	if (path[0] != '/')
-		return -EINVAL;
 	if (len >= PATH_MAX)
 		return -ENAMETOOLONG;
 
@@ -150,6 +148,15 @@ __bpf_kfunc int bpf_binprm_set_interp(struct linux_binprm *bprm,
 	if (!interp)
 		return -ENOMEM;
 
+	/*
+	 * The program may pass memory that is written to while this runs,
+	 * so check the private copy and not the buffer it was made from.
+	 */
+	if (interp[0] != '/') {
+		kfree(interp);
+		return -EINVAL;
+	}
+
 	bm_bpf_stage_selection(bprm, interp, NULL);
 	return 0;
 }
@@ -241,6 +248,15 @@ __bpf_kfunc int bpf_binprm_set_interp_arg(struct linux_binprm *bprm,
 	if (!val)
 		return -ENOMEM;
 
+	/*
+	 * The program may pass memory that is written to while this runs,
+	 * so check the private copy and not the buffer it was made from.
+	 */
+	if (!val[0]) {
+		kfree(val);
+		return -EINVAL;
+	}
+
 	kfree(bprm->bpf_interp_arg);
 	bprm->bpf_interp_arg = val;
 	return 0;

-- 
2.53.0