[bpf,1/2] bpf: fix NPE when computing changes_pkt_data of program w/o subprograms

Commit Message

Eduard Zingerman Dec. 12, 2024, 7:07 a.m. UTC

bpf_prog_aux->func field might be NULL if program does not have
subprograms except for main sub-program. The fixed commit does
bpf_prog_aux->func access unconditionally, which might lead to null
pointer dereference.

The bug could be triggered by replacing the following BPF program:

    SEC("tc")
    int main_changes(struct __sk_buff *sk)
    {
        bpf_skb_pull_data(sk, 0);
        return 0;
    }

With the following BPF program:

    SEC("freplace")
    long changes_pkt_data(struct __sk_buff *sk)
    {
        return bpf_skb_pull_data(sk, 0);
    }

bpf_prog_aux instance itself represents the main sub-program,
use this property to fix the bug.

Fixes: 81f6d0530ba0 ("bpf: check changes_pkt_data property for extension programs")
Reported-by: kernel test robot <[e-mail]>
Reported-by: Dan Carpenter <[e-mail]>
Closes: https://lore.kernel.org/r/[e-mail]/
Signed-off-by: Eduard Zingerman <[e-mail]>
---
 kernel/bpf/verifier.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Alexei Starovoitov Dec. 12, 2024, 5:30 p.m. UTC | #1

What is 'NPE' ?

Eduard Zingerman Dec. 12, 2024, 6:04 p.m. UTC | #2

On Thu, 2024-12-12 at 09:30 -0800, Alexei Starovoitov wrote:
> What is 'NPE' ?

Null Pointer Exception, java lingo, grepping through the kernel commit
logs does not seem it is used much. Can resubmit with changed subject.

[e-mail] Dec. 12, 2024, 7:50 p.m. UTC | #3

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <[e-mail]>:

On Wed, 11 Dec 2024 23:07:10 -0800 you wrote:
> bpf_prog_aux->func field might be NULL if program does not have
> subprograms except for main sub-program. The fixed commit does
> bpf_prog_aux->func access unconditionally, which might lead to null
> pointer dereference.
> 
> The bug could be triggered by replacing the following BPF program:
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: fix NPE when computing changes_pkt_data of program w/o subprograms
    https://git.kernel.org/bpf/bpf/c/ac6542ad9275
  - [bpf,2/2] selftests/bpf: extend changes_pkt_data with cases w/o subprograms
    https://git.kernel.org/bpf/bpf/c/04789af756a4

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c2e5d0e6e3d0..5e541339b2f6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22193,6 +22193,7 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 	}
 	if (tgt_prog) {
 		struct bpf_prog_aux *aux = tgt_prog->aux;
+		bool tgt_changes_pkt_data;
 
 		if (bpf_prog_is_dev_bound(prog->aux) &&
 		    !bpf_prog_dev_bound_match(prog, tgt_prog)) {
@@ -22227,8 +22228,10 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 					"Extension programs should be JITed\n");
 				return -EINVAL;
 			}
-			if (prog->aux->changes_pkt_data &&
-			    !aux->func[subprog]->aux->changes_pkt_data) {
+			tgt_changes_pkt_data = aux->func
+					       ? aux->func[subprog]->aux->changes_pkt_data
+					       : aux->changes_pkt_data;
+			if (prog->aux->changes_pkt_data && !tgt_changes_pkt_data) {
 				bpf_log(log,
 					"Extension program changes packet data, while original does not\n");
 				return -EINVAL;