netdev - Re: [PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking

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] [day] [month] [year] [list]

Message-Id: <20190702.140702.1594186047058268389.davem@davemloft.net>
Date:   Tue, 02 Jul 2019 14:07:02 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     sbrivio@...hat.com
Cc:     dsahern@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] ipv4: Fix off-by-one in route dump counter without
 netlink strict checking

From: Stefano Brivio <sbrivio@...hat.com>
Date: Sat, 29 Jun 2019 19:55:08 +0200

> In commit ee28906fd7a1 ("ipv4: Dump route exceptions if requested") I
> added a counter of per-node dumped routes (including actual routes and
> exceptions), analogous to the existing counter for dumped nodes. Dumping
> exceptions means we need to also keep track of how many routes are dumped
> for each node: this would be just one route per node, without exceptions.
> 
> When netlink strict checking is not enabled, we dump both routes and
> exceptions at the same time: the RTM_F_CLONED flag is not used as a
> filter. In this case, the per-node counter 'i_fa' is incremented by one
> to track the single dumped route, then also incremented by one for each
> exception dumped, and then stored as netlink callback argument as skip
> counter, 's_fa', to be used when a partial dump operation restarts.
> 
> The per-node counter needs to be increased by one also when we skip a
> route (exception) due to a previous non-zero skip counter, because it
> needs to match the existing skip counter, if we are dumping both routes
> and exceptions. I missed this, and only incremented the counter, for
> regular routes, if the previous skip counter was zero. This means that,
> in case of a mixed dump, partial dump operations after the first one
> will start with a mismatching skip counter value, one less than expected.
> 
> This means in turn that the first exception for a given node is skipped
> every time a partial dump operation restarts, if netlink strict checking
> is not enabled (iproute < 5.0).
> 
> It turns out I didn't repeat the test in its final version, commit
> de755a85130e ("selftests: pmtu: Introduce list_flush_ipv4_exception test
> case"), which also counts the number of route exceptions returned, with
> iproute2 versions < 5.0 -- I was instead using the equivalent of the IPv6
> test as it was before commit b964641e9925 ("selftests: pmtu: Make
> list_flush_ipv6_exception test more demanding").
> 
> Always increment the per-node counter by one if we previously dumped
> a regular route, so that it matches the current skip counter.
> 
> Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: Stefano Brivio <sbrivio@...hat.com>

Applied to net-next, thanks for fixing this.