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

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 4204 bytes --]

On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> 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 what happened here is that I came across this problem while
working on using payload expressions to set CT and packet marks, and in
that specific case stopping the double-evaluation in expr_evaluate_unary
fixed it.  However, when I looked for another example that was allowed
by the current grammar to put into the commit message, the one I found
was caused by stmt_evaluate_payload instead, but on the assumption that
the cause was the same, I didn't actually verify it.  Whoops.

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

Thanks for the analysis, Pablo.  I'll get started.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]