All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, John Fastabend <john.fastabend@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>
Subject: [PATCH 5.10 45/54] bpf: Fix 32 bit src register truncation on div/mod
Date: Thu, 11 Feb 2021 16:02:29 +0100	[thread overview]
Message-ID: <20210211150154.840544860@linuxfoundation.org> (raw)
In-Reply-To: <20210211150152.885701259@linuxfoundation.org>

From: Daniel Borkmann <daniel@iogearbox.net>

commit e88b2c6e5a4d9ce30d75391e4d950da74bb2bd90 upstream.

While reviewing a different fix, John and I noticed an oddity in one of the
BPF program dumps that stood out, for example:

  # bpftool p d x i 13
   0: (b7) r0 = 808464450
   1: (b4) w4 = 808464432
   2: (bc) w0 = w0
   3: (15) if r0 == 0x0 goto pc+1
   4: (9c) w4 %= w0
  [...]

In line 2 we noticed that the mov32 would 32 bit truncate the original src
register for the div/mod operation. While for the two operations the dst
register is typically marked unknown e.g. from adjust_scalar_min_max_vals()
the src register is not, and thus verifier keeps tracking original bounds,
simplified:

  0: R1=ctx(id=0,off=0,imm=0) R10=fp0
  0: (b7) r0 = -1
  1: R0_w=invP-1 R1=ctx(id=0,off=0,imm=0) R10=fp0
  1: (b7) r1 = -1
  2: R0_w=invP-1 R1_w=invP-1 R10=fp0
  2: (3c) w0 /= w1
  3: R0_w=invP(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=invP-1 R10=fp0
  3: (77) r1 >>= 32
  4: R0_w=invP(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=invP4294967295 R10=fp0
  4: (bf) r0 = r1
  5: R0_w=invP4294967295 R1_w=invP4294967295 R10=fp0
  5: (95) exit
  processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

Runtime result of r0 at exit is 0 instead of expected -1. Remove the
verifier mov32 src rewrite in div/mod and replace it with a jmp32 test
instead. After the fix, we result in the following code generation when
having dividend r1 and divisor r6:

  div, 64 bit:                             div, 32 bit:

   0: (b7) r6 = 8                           0: (b7) r6 = 8
   1: (b7) r1 = 8                           1: (b7) r1 = 8
   2: (55) if r6 != 0x0 goto pc+2           2: (56) if w6 != 0x0 goto pc+2
   3: (ac) w1 ^= w1                         3: (ac) w1 ^= w1
   4: (05) goto pc+1                        4: (05) goto pc+1
   5: (3f) r1 /= r6                         5: (3c) w1 /= w6
   6: (b7) r0 = 0                           6: (b7) r0 = 0
   7: (95) exit                             7: (95) exit

  mod, 64 bit:                             mod, 32 bit:

   0: (b7) r6 = 8                           0: (b7) r6 = 8
   1: (b7) r1 = 8                           1: (b7) r1 = 8
   2: (15) if r6 == 0x0 goto pc+1           2: (16) if w6 == 0x0 goto pc+1
   3: (9f) r1 %= r6                         3: (9c) w1 %= w6
   4: (b7) r0 = 0                           4: (b7) r0 = 0
   5: (95) exit                             5: (95) exit

x86 in particular can throw a 'divide error' exception for div
instruction not only for divisor being zero, but also for the case
when the quotient is too large for the designated register. For the
edx:eax and rdx:rax dividend pair it is not an issue in x86 BPF JIT
since we always zero edx (rdx). Hence really the only protection
needed is against divisor being zero.

Fixes: 68fda450a7df ("bpf: fix 32-bit divide by zero")
Co-developed-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 kernel/bpf/verifier.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10866,30 +10866,28 @@ static int fixup_bpf_calls(struct bpf_ve
 		    insn->code == (BPF_ALU | BPF_MOD | BPF_X) ||
 		    insn->code == (BPF_ALU | BPF_DIV | BPF_X)) {
 			bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
-			struct bpf_insn mask_and_div[] = {
-				BPF_MOV32_REG(insn->src_reg, insn->src_reg),
+			bool isdiv = BPF_OP(insn->code) == BPF_DIV;
+			struct bpf_insn *patchlet;
+			struct bpf_insn chk_and_div[] = {
 				/* Rx div 0 -> 0 */
-				BPF_JMP_IMM(BPF_JNE, insn->src_reg, 0, 2),
+				BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+					     BPF_JNE | BPF_K, insn->src_reg,
+					     0, 2, 0),
 				BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
 				BPF_JMP_IMM(BPF_JA, 0, 0, 1),
 				*insn,
 			};
-			struct bpf_insn mask_and_mod[] = {
-				BPF_MOV32_REG(insn->src_reg, insn->src_reg),
+			struct bpf_insn chk_and_mod[] = {
 				/* Rx mod 0 -> Rx */
-				BPF_JMP_IMM(BPF_JEQ, insn->src_reg, 0, 1),
+				BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+					     BPF_JEQ | BPF_K, insn->src_reg,
+					     0, 1, 0),
 				*insn,
 			};
-			struct bpf_insn *patchlet;
 
-			if (insn->code == (BPF_ALU64 | BPF_DIV | BPF_X) ||
-			    insn->code == (BPF_ALU | BPF_DIV | BPF_X)) {
-				patchlet = mask_and_div + (is64 ? 1 : 0);
-				cnt = ARRAY_SIZE(mask_and_div) - (is64 ? 1 : 0);
-			} else {
-				patchlet = mask_and_mod + (is64 ? 1 : 0);
-				cnt = ARRAY_SIZE(mask_and_mod) - (is64 ? 1 : 0);
-			}
+			patchlet = isdiv ? chk_and_div : chk_and_mod;
+			cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
+				      ARRAY_SIZE(chk_and_mod);
 
 			new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
 			if (!new_prog)



  parent reply	other threads:[~2021-02-11 15:37 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-11 15:01 [PATCH 5.10 00/54] 5.10.16-rc1 review Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 01/54] io_uring: simplify io_task_match() Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 02/54] io_uring: add a {task,files} pair matching helper Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 03/54] io_uring: dont iterate io_uring_cancel_files() Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 04/54] io_uring: pass files into kill timeouts/poll Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 05/54] io_uring: always batch cancel in *cancel_files() Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 06/54] io_uring: fix files cancellation Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 07/54] io_uring: account io_uring internal files as REQ_F_INFLIGHT Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 08/54] io_uring: if we see flush on exit, cancel related tasks Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 09/54] io_uring: fix __io_uring_files_cancel() with TASK_UNINTERRUPTIBLE Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 10/54] io_uring: replace inflight_wait with tctx->wait Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 11/54] io_uring: fix cancellation taking mutex while TASK_UNINTERRUPTIBLE Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 12/54] io_uring: fix flush cqring overflow list while TASK_INTERRUPTIBLE Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 13/54] io_uring: fix list corruption for splice file_get Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 14/54] io_uring: fix sqo ownership false positive warning Greg Kroah-Hartman
2021-02-11 15:01 ` [PATCH 5.10 15/54] io_uring: reinforce cancel on flush during exit Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 16/54] io_uring: drop mm/files between task_work_submit Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 17/54] gpiolib: cdev: clear debounce period if line set to output Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 18/54] powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 19/54] af_key: relax availability checks for skb size calculation Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 20/54] regulator: core: avoid regulator_resolve_supply() race condition Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 21/54] ASoC: wm_adsp: Fix control name parsing for multi-fw Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 22/54] drm/nouveau/nvif: fix method count when pushing an array Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 23/54] mac80211: 160MHz with extended NSS BW in CSA Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 24/54] ASoC: Intel: Skylake: Zero snd_ctl_elem_value Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 25/54] chtls: Fix potential resource leak Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 26/54] pNFS/NFSv4: Try to return invalid layout in pnfs_layout_process() Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 27/54] pNFS/NFSv4: Improve rejection of out-of-order layouts Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 28/54] ALSA: hda: intel-dsp-config: add PCI id for TGL-H Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 29/54] ASoC: ak4458: correct reset polarity Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 30/54] ASoC: Intel: sof_sdw: set proper flags for Dell TGL-H SKU 0A5E Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 31/54] iwlwifi: mvm: skip power command when unbinding vif during CSA Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 32/54] iwlwifi: mvm: take mutex for calling iwl_mvm_get_sync_time() Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 33/54] iwlwifi: pcie: add a NULL check in iwl_pcie_txq_unmap Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 34/54] iwlwifi: pcie: fix context info memory leak Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 35/54] iwlwifi: mvm: invalidate IDs of internal stations at mvm start Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 36/54] iwlwifi: pcie: add rules to match Qu with Hr2 Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 37/54] iwlwifi: mvm: guard against device removal in reprobe Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 38/54] iwlwifi: queue: bail out on invalid freeing Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 39/54] SUNRPC: Move simple_get_bytes and simple_get_netobj into private header Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 40/54] SUNRPC: Handle 0 length opaque XDR object data properly Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 41/54] i2c: mediatek: Move suspend and resume handling to NOIRQ phase Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 42/54] blk-cgroup: Use cond_resched() when destroy blkgs Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 43/54] regulator: Fix lockdep warning resolving supplies Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 44/54] bpf: Fix verifier jmp32 pruning decision logic Greg Kroah-Hartman
2021-02-11 15:02 ` Greg Kroah-Hartman [this message]
2021-02-11 15:02 ` [PATCH 5.10 46/54] bpf: Fix verifier jsgt branch analysis on max bound Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 47/54] drm/i915: Fix ICL MG PHY vswing handling Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 48/54] drm/i915: Skip vswing programming for TBT Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 49/54] nilfs2: make splice write available again Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 50/54] Revert "mm: memcontrol: avoid workload stalls when lowering memory.high" Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 51/54] squashfs: avoid out of bounds writes in decompressors Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 52/54] squashfs: add more sanity checks in id lookup Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 53/54] squashfs: add more sanity checks in inode lookup Greg Kroah-Hartman
2021-02-11 15:02 ` [PATCH 5.10 54/54] squashfs: add more sanity checks in xattr id lookup Greg Kroah-Hartman
2021-02-12  3:16 ` [PATCH 5.10 00/54] 5.10.16-rc1 review Naresh Kamboju
2021-02-13 12:58   ` Greg Kroah-Hartman
2021-02-12 16:17 ` Shuah Khan
2021-02-13 12:58   ` Greg Kroah-Hartman
2021-02-17 22:45     ` Shuah Khan
2021-02-12 18:08 ` Guenter Roeck
2021-02-13 12:58   ` Greg Kroah-Hartman
2021-02-12 19:21 ` Florian Fainelli
2021-02-13 12:58   ` Greg Kroah-Hartman
2021-02-12 19:54 ` Pavel Machek
2021-02-13 12:58   ` Greg Kroah-Hartman
2021-02-13  3:20 ` Ross Schmidt
2021-02-13 12:57   ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210211150154.840544860@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.