linux-kernel - Re: [RFC bpf-next 1/2] bpf: tnums: warn against the usage of tnum_in(tnum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxF984GIloJWnV9x@syu-laptop>
Date:   Fri, 2 Sep 2022 11:52:19 +0800
From:   Shung-Hsi Yu <shung-hsi.yu@...e.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [RFC bpf-next 1/2] bpf: tnums: warn against the usage of
 tnum_in(tnum_range(), ...)

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@...e.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
> > 
>