Bug 1477491

Summary: tc filter does not fail on malformed bytecode file
Product: Red Hat Enterprise Linux 7 Reporter: Jaroslav Aster <jaster>
Component: iprouteAssignee: Phil Sutter <psutter>
Status: CLOSED ERRATA QA Contact: Jaroslav Aster <jaster>
Severity: high Docs Contact:
Priority: high    
Version: 7.4-AltCC: atragler, psutter
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: iproute-4.11.0-3.el7a Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-11-09 11:25:03 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Regression reproducer. none

Description Jaroslav Aster 2017-08-02 09:09:49 UTC 
Description of problem:

tc filter does not fail on malformed bytecode file. It reads only first line, creates filter and ignores other line.


Version-Release number of selected component (if applicable):

iproute-4.11.0-1.el7a


How reproducible:

100%


Steps to Reproduce:

iproute-4.11.0-1.el7a

# cat /tmp/tmp.fejlMbWcco
7,40 0 0 12,21 0 4 2048,37 3 0 1500,48 0 0 14,21 0 1 66,6 0 0 262144,6 0 0 0,
12,12 13,13 0 4
blabla

# tc filter add dev TestIface parent 1:0 bpf run bytecode-file /tmp/tmp.fejlMbWcco flowid 1:13

# tc filter show dev TestIface | grep 'flowid 1:13'
filter parent 1: protocol all pref 49152 bpf handle 0x1 flowid 1:13 bytecode '7,40 0 0 12,21 0 4 2048,37 3 0 1500,48 0 0 14,21 0 1 66,6 0 0 262144,6 0 0 0'


iproute-3.10.0-87.el7

# cat /tmp/tmp.BgIfvpoxVL
7,40 0 0 12,21 0 4 2048,37 3 0 1500,48 0 0 14,21 0 1 66,6 0 0 262144,6 0 0 0,
12,12 13,13 0 4
blabla

# tc filter add dev TestIface parent 1:0 bpf run bytecode-file /tmp/tmp.BgIfvpoxVL flowid 1:13'
Real program length exceeds encoded length parameter!
Illegal "bytecode"

# tc filter show dev TestIface | grep 'flowid 1:13'
<nothing>


Actual results:

tc filter reads malformed bytecode file and creates filter instead of fails and creates nothing.


Expected results:

tc filter reports error, returns non-zero return code and creates nothing.
Comment 2 Phil Sutter 2017-08-02 13:23:33 UTC 
In fact, this is not a regression: The test case previously failed not because
of the invalid content on the second line, but because of the trailing newline
on the first one - the BPF parser never read more than a single line from the
provided bytecode file.

What has changed with the rebase is that the parser now removes any trailing
newline from that first line. This is a good thing per se, becase editors
commonly add that when editing a file (even if it contains just a single line)
anyway.

So in this particular case, the test case should be adjusted to append the
invalid bytecode part to the same line, not write a new line to the file.

Nevertheless I have submitted a patch upstream which enables the parser to
detect consecutive lines of bytecode and treat them correctly (which
implicitly also allows to split bytecode files over multiple lines):

http://marc.info/?l=linux-netdev&m=150167869213237&w=2

Cheers, Phil
Comment 3 Phil Sutter 2017-08-04 07:04:47 UTC 
Upstream accepted my patch:

commit 3da3ebfca85b8f1e8252b898453d8cb383c5c398
Author: Phil Sutter <phil>
Date:   Wed Aug 2 14:57:56 2017 +0200

    bpf: Make bytecode-file reading a little more robust
    
    bpf_parse_string() will now correctly handle:
    
    - Extraneous whitespace,
    - OPs on multiple lines and
    - overlong file names.
    
    The added feature of allowing to have OPs on multiple lines (like e.g.
    tcpdump prints them) is rather a side effect of fixing detection of
    malformed bytecode files having random content on a second line, like
    e.g.:
    
    | 4,40 0 0 12,21 0 1 2048,6 0 0 262144,6 0 0 0
    | foobar
    
    Cc: Daniel Borkmann <daniel>
    Signed-off-by: Phil Sutter <phil>
    Acked-by: Daniel Borkmann <daniel>
Comment 5 Jaroslav Aster 2017-08-28 15:03:25 UTC 
Hi Phil,

unfortunately I found a regression in the patch. It has stopped to work on correct bytecode file. See the reproducer I have attached.

iproute-4.11.0-1.el7a (old)

# cat /tmp/bytecode.txt 
7,40 0 0 12,21 0 4 2048,37 3 0 1500,48 0 0 14,21 0 1 66,6 0 0 262144,6 0 0 0,
# tc filter add dev TestIface parent 1:0 bpf run bytecode "$(cat /tmp/bytecode.txt)" flowid 1:11
# tc filter add dev TestIface parent 1:0 bpf run bytecode-file /tmp/bytecode.txt flowid 1:13


iproute-4.11.0-2.el7a (new)

# cat /tmp/bytecode.txt 
7,40 0 0 12,21 0 4 2048,37 3 0 1500,48 0 0 14,21 0 1 66,6 0 0 262144,6 0 0 0,
# tc filter add dev TestIface parent 1:0 bpf run bytecode "$(cat /tmp/bytecode.txt)" flowid 1:11
# tc filter add dev TestIface parent 1:0 bpf run bytecode-file /tmp/bytecode.txt flowid 1:13
Real program length exceeds encoded length parameter!
Comment 6 Jaroslav Aster 2017-08-28 15:04:14 UTC 
Created attachment 1319118 [details]
Regression reproducer.
Comment 7 Phil Sutter 2017-08-29 15:15:21 UTC 
Bug found, seems to happen on certain architectures only. Fix submitted upstream:
https://marc.info/?l=linux-netdev&m=150401943515102&w=2
Comment 10 errata-xmlrpc 2017-11-09 11:25:03 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:3172