diff mbox series

[bpf] xdp: use trusted arguments in XDP hints kfuncs

Message ID 20230711105930.29170-1-larysa.zaremba@intel.com (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series [bpf] xdp: use trusted arguments in XDP hints kfuncs | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-6 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-26 success Logs for test_verifier on s390x with gcc
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1345 this patch: 1345
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1368 this patch: 1368
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for test_maps on s390x with gcc

Commit Message

Larysa Zaremba July 11, 2023, 10:59 a.m. UTC 
Currently, verifier does not reject XDP programs that pass NULL pointer to
hints functions. At the same time, this case is not handled in any driver
implementation (including veth). For example, changing

bpf_xdp_metadata_rx_timestamp(ctx, &timestamp);

to

bpf_xdp_metadata_rx_timestamp(ctx, NULL);

in xdp_metadata test successfully crashes the system.

Add KF_TRUSTED_ARGS flag to hints kfunc definitions, so driver code
does not have to worry about getting invalid pointers.

Fixes: 3d76a4d3d4e5 ("bpf: XDP metadata RX kfuncs")
Reported-by: Stanislav Fomichev <sdf@google.com>
Closes: https://lore.kernel.org/bpf/ZKWo0BbpLfkZHbyE@google.com/
Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
---
 net/core/xdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jesper Dangaard Brouer July 11, 2023, 2:21 p.m. UTC | #1 
On 11/07/2023 12.59, Larysa Zaremba wrote:
> Currently, verifier does not reject XDP programs that pass NULL pointer to
> hints functions. At the same time, this case is not handled in any driver
> implementation (including veth). For example, changing
> 
> bpf_xdp_metadata_rx_timestamp(ctx, &timestamp);
> 
> to
> 
> bpf_xdp_metadata_rx_timestamp(ctx, NULL);
> 
> in xdp_metadata test successfully crashes the system.
> 
> Add KF_TRUSTED_ARGS flag to hints kfunc definitions, so driver code
> does not have to worry about getting invalid pointers.
> 

Looks good to me, assuming this means verifier will reject BPF-prog's 
supplying NULL.

Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>

> Fixes: 3d76a4d3d4e5 ("bpf: XDP metadata RX kfuncs")
> Reported-by: Stanislav Fomichev <sdf@google.com>
> Closes: https://lore.kernel.org/bpf/ZKWo0BbpLfkZHbyE@google.com/
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
>   net/core/xdp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 41e5ca8643ec..8362130bf085 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -741,7 +741,7 @@ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
>   __diag_pop();
>   
>   BTF_SET8_START(xdp_metadata_kfunc_ids)
> -#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, 0)
> +#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, KF_TRUSTED_ARGS)
>   XDP_METADATA_KFUNC_xxx
>   #undef XDP_METADATA_KFUNC
>   BTF_SET8_END(xdp_metadata_kfunc_ids)
Stanislav Fomichev July 11, 2023, 5 p.m. UTC | #2 
On Tue, Jul 11, 2023 at 7:21 AM Jesper Dangaard Brouer
<jbrouer@redhat.com> wrote:
>
>
> On 11/07/2023 12.59, Larysa Zaremba wrote:
> > Currently, verifier does not reject XDP programs that pass NULL pointer to
> > hints functions. At the same time, this case is not handled in any driver
> > implementation (including veth). For example, changing
> >
> > bpf_xdp_metadata_rx_timestamp(ctx, &timestamp);
> >
> > to
> >
> > bpf_xdp_metadata_rx_timestamp(ctx, NULL);
> >
> > in xdp_metadata test successfully crashes the system.
> >
> > Add KF_TRUSTED_ARGS flag to hints kfunc definitions, so driver code
> > does not have to worry about getting invalid pointers.
> >
>
> Looks good to me, assuming this means verifier will reject BPF-prog's
> supplying NULL.
>
> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
>
> > Fixes: 3d76a4d3d4e5 ("bpf: XDP metadata RX kfuncs")
> > Reported-by: Stanislav Fomichev <sdf@google.com>
> > Closes: https://lore.kernel.org/bpf/ZKWo0BbpLfkZHbyE@google.com/
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>

Acked-by: Stanislav Fomichev <sdf@google.com>

Thank you for the fix!

> > ---
> >   net/core/xdp.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 41e5ca8643ec..8362130bf085 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -741,7 +741,7 @@ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
> >   __diag_pop();
> >
> >   BTF_SET8_START(xdp_metadata_kfunc_ids)
> > -#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, 0)
> > +#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, KF_TRUSTED_ARGS)
> >   XDP_METADATA_KFUNC_xxx
> >   #undef XDP_METADATA_KFUNC
> >   BTF_SET8_END(xdp_metadata_kfunc_ids)
>
Alexei Starovoitov July 12, 2023, 3:06 a.m. UTC | #3 
On Tue, Jul 11, 2023 at 10:00 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Tue, Jul 11, 2023 at 7:21 AM Jesper Dangaard Brouer
> <jbrouer@redhat.com> wrote:
> >
> >
> > On 11/07/2023 12.59, Larysa Zaremba wrote:
> > > Currently, verifier does not reject XDP programs that pass NULL pointer to
> > > hints functions. At the same time, this case is not handled in any driver
> > > implementation (including veth). For example, changing
> > >
> > > bpf_xdp_metadata_rx_timestamp(ctx, &timestamp);
> > >
> > > to
> > >
> > > bpf_xdp_metadata_rx_timestamp(ctx, NULL);
> > >
> > > in xdp_metadata test successfully crashes the system.
> > >
> > > Add KF_TRUSTED_ARGS flag to hints kfunc definitions, so driver code
> > > does not have to worry about getting invalid pointers.
> > >
> >
> > Looks good to me, assuming this means verifier will reject BPF-prog's
> > supplying NULL.
> >
> > Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
> >
> > > Fixes: 3d76a4d3d4e5 ("bpf: XDP metadata RX kfuncs")
> > > Reported-by: Stanislav Fomichev <sdf@google.com>
> > > Closes: https://lore.kernel.org/bpf/ZKWo0BbpLfkZHbyE@google.com/
> > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
>
> Acked-by: Stanislav Fomichev <sdf@google.com>
>
> Thank you for the fix!

Applied. Thanks
diff mbox series

Patch

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 41e5ca8643ec..8362130bf085 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -741,7 +741,7 @@  __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
 __diag_pop();
 
 BTF_SET8_START(xdp_metadata_kfunc_ids)
-#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, 0)
+#define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, KF_TRUSTED_ARGS)
 XDP_METADATA_KFUNC_xxx
 #undef XDP_METADATA_KFUNC
 BTF_SET8_END(xdp_metadata_kfunc_ids)