Patch "bpf: Remove misleading spec_v1 check on var-offset stack read" has been added to the 5.10-stable tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This is a note to let you know that I've just added the patch titled

    bpf: Remove misleading spec_v1 check on var-offset stack read

to the 5.10-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     bpf-remove-misleading-spec_v1-check-on-var-offset-st.patch
and it can be found in the queue-5.10 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.



commit 138524c236d9c1d26566161838630a1c29db15f2
Author: Luis Gerhorst <gerhorst@xxxxxxxxx>
Date:   Wed Mar 15 17:54:00 2023 +0100

    bpf: Remove misleading spec_v1 check on var-offset stack read
    
    [ Upstream commit 082cdc69a4651dd2a77539d69416a359ed1214f5 ]
    
    For every BPF_ADD/SUB involving a pointer, adjust_ptr_min_max_vals()
    ensures that the resulting pointer has a constant offset if
    bypass_spec_v1 is false. This is ensured by calling sanitize_check_bounds()
    which in turn calls check_stack_access_for_ptr_arithmetic(). There,
    -EACCESS is returned if the register's offset is not constant, thereby
    rejecting the program.
    
    In summary, an unprivileged user must never be able to create stack
    pointers with a variable offset. That is also the case, because a
    respective check in check_stack_write() is missing. If they were able
    to create a variable-offset pointer, users could still use it in a
    stack-write operation to trigger unsafe speculative behavior [1].
    
    Because unprivileged users must already be prevented from creating
    variable-offset stack pointers, viable options are to either remove
    this check (replacing it with a clarifying comment), or to turn it
    into a "verifier BUG"-message, also adding a similar check in
    check_stack_write() (for consistency, as a second-level defense).
    This patch implements the first option to reduce verifier bloat.
    
    This check was introduced by commit 01f810ace9ed ("bpf: Allow
    variable-offset stack access") which correctly notes that
    "variable-offset reads and writes are disallowed (they were already
    disallowed for the indirect access case) because the speculative
    execution checking code doesn't support them". However, it does not
    further discuss why the check in check_stack_read() is necessary.
    The code which made this check obsolete was also introduced in this
    commit.
    
    I have compiled ~650 programs from the Linux selftests, Linux samples,
    Cilium, and libbpf/examples projects and confirmed that none of these
    trigger the check in check_stack_read() [2]. Instead, all of these
    programs are, as expected, already rejected when constructing the
    variable-offset pointers. Note that the check in
    check_stack_access_for_ptr_arithmetic() also prints "off=%d" while the
    code removed by this patch does not (the error removed does not appear
    in the "verification_error" values). For reproducibility, the
    repository linked includes the raw data and scripts used to create
    the plot.
    
      [1] https://arxiv.org/pdf/1807.03757.pdf
      [2] https://gitlab.cs.fau.de/un65esoq/bpf-spectre/-/raw/53dc19fcf459c186613b1156a81504b39c8d49db/data/plots/23-02-26_23-56_bpftool/bpftool/0004-errors.pdf?inline=false
    
    Fixes: 01f810ace9ed ("bpf: Allow variable-offset stack access")
    Signed-off-by: Luis Gerhorst <gerhorst@xxxxxxxxx>
    Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
    Acked-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
    Link: https://lore.kernel.org/bpf/20230315165358.23701-1-gerhorst@xxxxxxxxx
    Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 232f720104cdf..6876796a8de0c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2775,17 +2775,13 @@ static int check_stack_read(struct bpf_verifier_env *env,
 	}
 	/* Variable offset is prohibited for unprivileged mode for simplicity
 	 * since it requires corresponding support in Spectre masking for stack
-	 * ALU. See also retrieve_ptr_limit().
+	 * ALU. See also retrieve_ptr_limit(). The check in
+	 * check_stack_access_for_ptr_arithmetic() called by
+	 * adjust_ptr_min_max_vals() prevents users from creating stack pointers
+	 * with variable offsets, therefore no check is required here. Further,
+	 * just checking it here would be insufficient as speculative stack
+	 * writes could still lead to unsafe speculative behaviour.
 	 */
-	if (!env->bypass_spec_v1 && var_off) {
-		char tn_buf[48];
-
-		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
-		verbose(env, "R%d variable offset stack access prohibited for !root, var_off=%s\n",
-				ptr_regno, tn_buf);
-		return -EACCES;
-	}
-
 	if (!var_off) {
 		off += reg->var_off.value;
 		err = check_stack_read_fixed_off(env, state, off, size,



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux