[PATCH bpf-next 0/2] bpf: Allow doing stack read with size larger than the earlier spilled reg

[PATCH bpf-next 0/2] bpf: Allow doing stack read with size larger than the earlier spilled reg

[Martin KaFai Lau]

This set fixes an issue that the verifier rejects a u64 stack read
after an earlier u32 scalar spill.  It is caused by the earlier commit
that allows tracking the spilled u32 scalar reg.

Martin KaFai Lau (2):
  bpf: Do not reject when the stack read size is different from the
    tracked scalar size
  bpf: selftest: verifier test on refill from a smaller spill

 kernel/bpf/verifier.c                          | 18 ++++++------------
 .../selftests/bpf/verifier/spill_fill.c        | 17 +++++++++++++++++
 2 files changed, 23 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/2] bpf: Do not reject when the stack read size is different from the tracked scalar size

[Martin KaFai Lau]

Below is a simplified case from a report in bcc [0]:
r4 = 20
*(u32 *)(r10 -4) = r4
*(u32 *)(r10 -8) = r4  /* r4 state is tracked */
r4 = *(u64 *)(r10 -8)  /* Read more than the tracked 32bit scalar.
			* verifier rejects as 'corrupted spill memory'.
			*/

After commit 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill"),
the 8-byte aligned 32bit spill is also tracked by the verifier
and the reg state is stored.

However, if 8 bytes are read from the stack instead of the tracked
4 byte scalar, the verifier currently rejects as "corrupted spill memory".

This patch fixes this case by allowing it to read but marks the reg as
unknown.

Also note that, if the prog is trying to corrupt/leak an
earlier spilled pointer by spilling another <8 bytes register on top,
this has already been rejected in the check_stack_write_fixed_off().

[0]: https://github.com/iovisor/bcc/pull/3683

Fixes: 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill")
Reported-by: Hengqi Chen <hengqi.chen@gmail.com>
Reported-by: Yonghong Song <yhs@gmail.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 kernel/bpf/verifier.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3c8aa7df1773..d8012775831d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3088,9 +3088,12 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
 	reg = &reg_state->stack[spi].spilled_ptr;
 
 	if (is_spilled_reg(&reg_state->stack[spi])) {
-		if (size != BPF_REG_SIZE) {
-			u8 scalar_size = 0;
+		u8 spill_size = 1;
+
+		for (i = BPF_REG_SIZE - 1; i > 0 && stype[i - 1] == STACK_SPILL; i--)
+			spill_size++;
 
+		if (size != BPF_REG_SIZE || spill_size != BPF_REG_SIZE) {
 			if (reg->type != SCALAR_VALUE) {
 				verbose_linfo(env, env->insn_idx, "; ");
 				verbose(env, "invalid size of register fill\n");
@@ -3101,10 +3104,7 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
 			if (dst_regno < 0)
 				return 0;
 
-			for (i = BPF_REG_SIZE; i > 0 && stype[i - 1] == STACK_SPILL; i--)
-				scalar_size++;
-
-			if (!(off % BPF_REG_SIZE) && size == scalar_size) {
+			if (!(off % BPF_REG_SIZE) && size == spill_size) {
 				/* The earlier check_reg_arg() has decided the
 				 * subreg_def for this insn.  Save it first.
 				 */
@@ -3128,12 +3128,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
 			state->regs[dst_regno].live |= REG_LIVE_WRITTEN;
 			return 0;
 		}
-		for (i = 1; i < BPF_REG_SIZE; i++) {
-			if (stype[(slot - i) % BPF_REG_SIZE] != STACK_SPILL) {
-				verbose(env, "corrupted spill memory\n");
-				return -EACCES;
-			}
-		}
 
 		if (dst_regno >= 0) {
 			/* restore register state from stack */
-- 
2.30.2

Re: [PATCH bpf-next 1/2] bpf: Do not reject when the stack read size is different from the tracked scalar size

[Hengqi Chen]

Hi, Martin.

On 2021/11/2 2:45 PM, Martin KaFai Lau wrote:
> Below is a simplified case from a report in bcc [0]:
> r4 = 20
> *(u32 *)(r10 -4) = r4
> *(u32 *)(r10 -8) = r4  /* r4 state is tracked */
> r4 = *(u64 *)(r10 -8)  /* Read more than the tracked 32bit scalar.
> 			* verifier rejects as 'corrupted spill memory'.
> 			*/
> 
> After commit 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill"),
> the 8-byte aligned 32bit spill is also tracked by the verifier
> and the reg state is stored.
> 
> However, if 8 bytes are read from the stack instead of the tracked
> 4 byte scalar, the verifier currently rejects as "corrupted spill memory".
> 
> This patch fixes this case by allowing it to read but marks the reg as
> unknown.
> 
> Also note that, if the prog is trying to corrupt/leak an
> earlier spilled pointer by spilling another <8 bytes register on top,
> this has already been rejected in the check_stack_write_fixed_off().
> 
> [0]: https://github.com/iovisor/bcc/pull/3683
> 
> Fixes: 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill")
> Reported-by: Hengqi Chen <hengqi.chen@gmail.com>
> Reported-by: Yonghong Song <yhs@gmail.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---

[...]

Thanks for the quick fix. I've tested this patch and now the BCC tools work fine.

Tested-by: Hengqi Chen <hengqi.chen@gmail.com>

Cheers,
--
Hengqi

Re: [PATCH bpf-next 1/2] bpf: Do not reject when the stack read size is different from the tracked scalar size

[Yonghong Song]

On 11/1/21 11:45 PM, Martin KaFai Lau wrote:
> Below is a simplified case from a report in bcc [0]:
> r4 = 20
> *(u32 *)(r10 -4) = r4
> *(u32 *)(r10 -8) = r4  /* r4 state is tracked */
> r4 = *(u64 *)(r10 -8)  /* Read more than the tracked 32bit scalar.
> 			* verifier rejects as 'corrupted spill memory'.
> 			*/
> 
> After commit 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill"),
> the 8-byte aligned 32bit spill is also tracked by the verifier
> and the reg state is stored.
> 
> However, if 8 bytes are read from the stack instead of the tracked
> 4 byte scalar, the verifier currently rejects as "corrupted spill memory".
> 
> This patch fixes this case by allowing it to read but marks the reg as
> unknown.
> 
> Also note that, if the prog is trying to corrupt/leak an
> earlier spilled pointer by spilling another <8 bytes register on top,
> this has already been rejected in the check_stack_write_fixed_off().
> 
> [0]: https://github.com/iovisor/bcc/pull/3683
> 
> Fixes: 354e8f1970f8 ("bpf: Support <8-byte scalar spill and refill")
> Reported-by: Hengqi Chen <hengqi.chen@gmail.com>
> Reported-by: Yonghong Song <yhs@gmail.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

[PATCH bpf-next 2/2] bpf: selftest: verifier test on refill from a smaller spill

[Martin KaFai Lau]

This patch adds a verifier test to ensure the verifier
can read 8 bytes from the stack after two 32bit write at
fp-4 and fp-8.  The test is similar to the reported case from bcc [0].

[0]: https://github.com/iovisor/bcc/pull/3683

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 .../testing/selftests/bpf/verifier/spill_fill.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index c9991c3f3bd2..7ab3de108761 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -265,3 +265,20 @@
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 },
+{
+	"Spill a u32 scalar at fp-4 and then at fp-8",
+	.insns = {
+	/* r4 = 4321 */
+	BPF_MOV32_IMM(BPF_REG_4, 4321),
+	/* *(u32 *)(r10 -4) = r4 */
+	BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_4, -4),
+	/* *(u32 *)(r10 -8) = r4 */
+	BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_4, -8),
+	/* r4 = *(u64 *)(r10 -8) */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_4, BPF_REG_10, -8),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+},
-- 
2.30.2

Re: [PATCH bpf-next 2/2] bpf: selftest: verifier test on refill from a smaller spill

[Yonghong Song]

On 11/1/21 11:45 PM, Martin KaFai Lau wrote:
> This patch adds a verifier test to ensure the verifier
> can read 8 bytes from the stack after two 32bit write at
> fp-4 and fp-8.  The test is similar to the reported case from bcc [0].
> 
> [0]: https://github.com/iovisor/bcc/pull/3683
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next 0/2] bpf: Allow doing stack read with size larger than the earlier spilled reg

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 1 Nov 2021 23:45:28 -0700 you wrote:
> This set fixes an issue that the verifier rejects a u64 stack read
> after an earlier u32 scalar spill.  It is caused by the earlier commit
> that allows tracking the spilled u32 scalar reg.
> 
> Martin KaFai Lau (2):
>   bpf: Do not reject when the stack read size is different from the
>     tracked scalar size
>   bpf: selftest: verifier test on refill from a smaller spill
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: Do not reject when the stack read size is different from the tracked scalar size
    https://git.kernel.org/bpf/bpf/c/f30d4968e9ae
  - [bpf-next,2/2] bpf: selftest: verifier test on refill from a smaller spill
    https://git.kernel.org/bpf/bpf/c/c08455dec5ac

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html