Re: [PATCH nft] evaluate: don't eval unary arguments.

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Jeremy,

On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > When a unary expression is inserted to implement a byte-order
> > > conversion, the expression being converted has already been
> > > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > > most types of expression, the double evaluation doesn't matter since
> > > evaluation is idempotent.  However, in the case of payload
> > > expressions which are munged during evaluation, it can cause
> > > unexpected errors:
> > >
> > >   # nft add table ip t
> > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > >   Error: Value 252 exceeds valid range 0-63
> > >   add rule ip t c ip dscp set ip dscp | 0x10
> > >                               ^^^^^^^
> >
> > I'm still hitting this after applying this patch.
> >
> > nft add rule ip t c ip dscp set ip dscp or 0x10
> > Error: Value 252 exceeds valid range 0-63
> > add rule ip t c ip dscp set ip dscp or 0x10
> >                             ^^^^^^
> > Probably problem is somewhere else? I'm not sure why we can assume
> > here that the argument of the unary expression should not be
> > evaluated.
> 
> I'll take another look.

I think stmt_evaluate_payload() is incomplete, this function was not
made to deal with non-constant expression as values.

Look:
        tcp dport set tcp sport

works because it follows the 'easy path', ie. no adjustment to make
the checksum calculation happy (see payload_needs_adjustment() in
stmt_evaluate_payload().

However:

        ip dscp set ip dscp

bails out with:

        nft add rule ip t c ip dscp set ip dscp
        Error: Value 252 exceeds valid range 0-63
        add rule ip t c ip dscp set ip dscp
                                    ^^^^^^^

because this follows the more complicated path. Looking at this code,
this path assumes a constant value, ie. ip dscp set 10. A more complex
thing such a non-constant expression (as in the example above) will
need a bit of work.

Probably you can start making a patchset make this work:

        add rule ip t c tcp dport set tcp dport lshift 1

which triggers:

BUG: invalid binary operation 4
nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0' failed.

since it's missing the bytecode to generate the left-shift. Not very
useful for users, but we can get something already merged upstream and
you'll be half-way done. Merge also a few tests.

Then, once the more fundamental rshift/lshift bits are merged, look at
this 'harder' path. Just a proposal.

For reference, the expression tree that stmt_evaluate_payload() to
make the checksum adjustment looks like this:

           xor
          /   \
        and   value
        / \
payload_   mask
 bytes

payload_bytes extends the payload expression to get up to 16-bits.
The left hand side is there to fetch bits that need to be left
untouched. The right hand side represent the bits that need to be set.

In the new non-constant scenario, the 'value' tree is actually a
binary operation:

         shift
        /    \
   payload   imm

The unary should not really be there, it's likely related to some
incorrect byteorder issue that kicks in with non-constant expression.

So more work on stmt_evaluate_payload() is required.