| Message ID | 20220831031907.16133-2-shung-hsi.yu@suse.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: tnums: warn against the usage of tnum_in(tnum_range(), ...) | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 33 this patch: 33 |
| netdev/cc_maintainers | success | CCed 2 of 2 maintainers |
| netdev/build_clang | success | Errors and warnings before: 17 this patch: 17 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 33 this patch: 33 |
| netdev/checkpatch | warning | WARNING: Unknown commit id 'a657182a5c51', maybe rebased or not pulled? |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | pending | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | pending | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | pending | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-12 | pending | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | pending | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | pending | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
On 8/31/22 5:19 AM, Shung-Hsi Yu wrote: > Commit a657182a5c51 ("bpf: Don't use tnum_range on array range checking > for poke descriptors") has shown that using tnum_range() as argument to > tnum_in() can lead to misleading code that looks like tight bound check > when in fact the actual allowed range is much wider. > > Document such behavior to warn against its usage in general, and suggest > some scenario where result can be trusted. > > Link: https://lore.kernel.org/bpf/984b37f9fdf7ac36831d2137415a4a915744c1b6.1661462653.git.daniel@iogearbox.net/ > Link: https://www.openwall.com/lists/oss-security/2022/08/26/1 > Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> Any objections from your side if I merge this? Thanks for adding doc. :) > --- > include/linux/tnum.h | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/include/linux/tnum.h b/include/linux/tnum.h > index 498dbcedb451..0ec4cda9e174 100644 > --- a/include/linux/tnum.h > +++ b/include/linux/tnum.h > @@ -21,7 +21,12 @@ struct tnum { > struct tnum tnum_const(u64 value); > /* A completely unknown value */ > extern const struct tnum tnum_unknown; > -/* A value that's unknown except that @min <= value <= @max */ > +/* An unknown value that is a superset of @min <= value <= @max. > + * > + * Could including values outside the range of [@min, @max]. > + * For example tnum_range(0, 2) is represented by {0, 1, 2, *3*}, rather than > + * the intended set of {0, 1, 2}. > + */ > struct tnum tnum_range(u64 min, u64 max); > > /* Arithmetic and logical ops */ > @@ -73,7 +78,18 @@ static inline bool tnum_is_unknown(struct tnum a) > */ > bool tnum_is_aligned(struct tnum a, u64 size); > > -/* Returns true if @b represents a subset of @a. */ > +/* Returns true if @b represents a subset of @a. > + * > + * Note that using tnum_range() as @a requires extra cautions as tnum_in() may > + * return true unexpectedly due to tnum limited ability to represent tight > + * range, e.g. > + * > + * tnum_in(tnum_range(0, 2), tnum_const(3)) == true > + * > + * As a rule of thumb, if @a is explicitly coded rather than coming from > + * reg->var_off, it should be in form of tnum_const(), tnum_range(0, 2**n - 1), > + * or tnum_range(2**n, 2**(n+1) - 1). > + */ > bool tnum_in(struct tnum a, struct tnum b); > > /* Formatting functions. These have snprintf-like semantics: they will write >
On Thu, Sep 01, 2022 at 05:00:58PM +0200, Daniel Borkmann wrote: > On 8/31/22 5:19 AM, Shung-Hsi Yu wrote: > > Commit a657182a5c51 ("bpf: Don't use tnum_range on array range checking > > for poke descriptors") has shown that using tnum_range() as argument to > > tnum_in() can lead to misleading code that looks like tight bound check > > when in fact the actual allowed range is much wider. > > > > Document such behavior to warn against its usage in general, and suggest > > some scenario where result can be trusted. > > > > Link: https://lore.kernel.org/bpf/984b37f9fdf7ac36831d2137415a4a915744c1b6.1661462653.git.daniel@iogearbox.net/ > > Link: https://www.openwall.com/lists/oss-security/2022/08/26/1 > > Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> > > Any objections from your side if I merge this? Thanks for adding doc. :) There is a small typo I meant to fix with s/including/include below. Other than that, none at all, thanks! :) > > --- > > include/linux/tnum.h | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/tnum.h b/include/linux/tnum.h > > index 498dbcedb451..0ec4cda9e174 100644 > > --- a/include/linux/tnum.h > > +++ b/include/linux/tnum.h > > @@ -21,7 +21,12 @@ struct tnum { > > struct tnum tnum_const(u64 value); > > /* A completely unknown value */ > > extern const struct tnum tnum_unknown; > > -/* A value that's unknown except that @min <= value <= @max */ > > +/* An unknown value that is a superset of @min <= value <= @max. > > + * > > + * Could including values outside the range of [@min, @max]. ^^^^^^^^^ include > > + * For example tnum_range(0, 2) is represented by {0, 1, 2, *3*}, rather than > > + * the intended set of {0, 1, 2}. > > + */ > > struct tnum tnum_range(u64 min, u64 max); > > /* Arithmetic and logical ops */ > > @@ -73,7 +78,18 @@ static inline bool tnum_is_unknown(struct tnum a) > > */ > > bool tnum_is_aligned(struct tnum a, u64 size); > > -/* Returns true if @b represents a subset of @a. */ > > +/* Returns true if @b represents a subset of @a. > > + * > > + * Note that using tnum_range() as @a requires extra cautions as tnum_in() may > > + * return true unexpectedly due to tnum limited ability to represent tight > > + * range, e.g. > > + * > > + * tnum_in(tnum_range(0, 2), tnum_const(3)) == true > > + * > > + * As a rule of thumb, if @a is explicitly coded rather than coming from > > + * reg->var_off, it should be in form of tnum_const(), tnum_range(0, 2**n - 1), > > + * or tnum_range(2**n, 2**(n+1) - 1). > > + */ > > bool tnum_in(struct tnum a, struct tnum b); > > /* Formatting functions. These have snprintf-like semantics: they will write > > >
On 9/2/22 5:52 AM, Shung-Hsi Yu wrote: > On Thu, Sep 01, 2022 at 05:00:58PM +0200, Daniel Borkmann wrote: >> On 8/31/22 5:19 AM, Shung-Hsi Yu wrote: >>> Commit a657182a5c51 ("bpf: Don't use tnum_range on array range checking >>> for poke descriptors") has shown that using tnum_range() as argument to >>> tnum_in() can lead to misleading code that looks like tight bound check >>> when in fact the actual allowed range is much wider. >>> >>> Document such behavior to warn against its usage in general, and suggest >>> some scenario where result can be trusted. >>> >>> Link: https://lore.kernel.org/bpf/984b37f9fdf7ac36831d2137415a4a915744c1b6.1661462653.git.daniel@iogearbox.net/ >>> Link: https://www.openwall.com/lists/oss-security/2022/08/26/1 >>> Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> >> >> Any objections from your side if I merge this? Thanks for adding doc. :) > > There is a small typo I meant to fix with s/including/include below. > > Other than that, none at all, thanks! :) Fixed up and applied to bpf-next, thanks!
diff --git a/include/linux/tnum.h b/include/linux/tnum.h index 498dbcedb451..0ec4cda9e174 100644 --- a/include/linux/tnum.h +++ b/include/linux/tnum.h @@ -21,7 +21,12 @@ struct tnum { struct tnum tnum_const(u64 value); /* A completely unknown value */ extern const struct tnum tnum_unknown; -/* A value that's unknown except that @min <= value <= @max */ +/* An unknown value that is a superset of @min <= value <= @max. + * + * Could including values outside the range of [@min, @max]. + * For example tnum_range(0, 2) is represented by {0, 1, 2, *3*}, rather than + * the intended set of {0, 1, 2}. + */ struct tnum tnum_range(u64 min, u64 max); /* Arithmetic and logical ops */ @@ -73,7 +78,18 @@ static inline bool tnum_is_unknown(struct tnum a) */ bool tnum_is_aligned(struct tnum a, u64 size); -/* Returns true if @b represents a subset of @a. */ +/* Returns true if @b represents a subset of @a. + * + * Note that using tnum_range() as @a requires extra cautions as tnum_in() may + * return true unexpectedly due to tnum limited ability to represent tight + * range, e.g. + * + * tnum_in(tnum_range(0, 2), tnum_const(3)) == true + * + * As a rule of thumb, if @a is explicitly coded rather than coming from + * reg->var_off, it should be in form of tnum_const(), tnum_range(0, 2**n - 1), + * or tnum_range(2**n, 2**(n+1) - 1). + */ bool tnum_in(struct tnum a, struct tnum b); /* Formatting functions. These have snprintf-like semantics: they will write
Commit a657182a5c51 ("bpf: Don't use tnum_range on array range checking for poke descriptors") has shown that using tnum_range() as argument to tnum_in() can lead to misleading code that looks like tight bound check when in fact the actual allowed range is much wider. Document such behavior to warn against its usage in general, and suggest some scenario where result can be trusted. Link: https://lore.kernel.org/bpf/984b37f9fdf7ac36831d2137415a4a915744c1b6.1661462653.git.daniel@iogearbox.net/ Link: https://www.openwall.com/lists/oss-security/2022/08/26/1 Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> --- include/linux/tnum.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)