Summary: | IPFW keep-state rules with untag do not go through parent rule cmd | ||
---|---|---|---|
Product: | Base System | Reporter: | fodillemlinkarim |
Component: | kern | Assignee: | John Baldwin <jhb> |
Status: | Closed FIXED | ||
Severity: | Affects Some People | CC: | ae, emaste, jhb |
Priority: | --- | Flags: | jhb:
mfc-stable14+
jhb: mfc-stable13+ |
Version: | CURRENT | ||
Hardware: | Any | ||
OS: | Any |
Description
fodillemlinkarim
2024-01-30 13:55:13 UTC
I agree with the diagnosis. I suspect though that the bug is a bit bigger as currently we always skip over the first bug (Hit Enter too soon, ignore previous comment) I agree with the diagnosis. I suspect though that the bug is a bit bigger as currently we always skip over the first action opcode. The fact that 'match' is set to 1 allows this to "work" if the first action is "accept" which is usually the action for keep-state rules. However, I suspect that if you have a 'log' action on a keep-state rule we don't actually log packets that match an existing dynamic rule since we skip over the "log" opcode due to this bug. A bit more background: in this set of loops in the kernel, you can think of 'cmd' as being a program counter (PC) for an ISA and 'cmdlen' is the implicit PC increment to perform after handling the current opcode. Since this action is triggering the equivalent of a branch, it resets 'cmd' and 'l' as is done at the start of the inner for loop and sets 'cmdlen' to 0 to avoid turn the implicit PC increment at the end of the for loop into a nop. I think though that the patch should drop the 'match = 1' as that is now just noise. Also, there is no need to keep the dead 'break' statement. I've cc'd ae@ to see if he has any thoughts, but if there's no other feedback in the next week or so I'll commit the tweaked fix. Fine by me, the break and match were left there to stay consistent with other parts of the that file that behave in a similar fashion, for example the O_COUNT and O_SKIPTO cases. (In reply to John Baldwin from comment #2) I agree. I just hope the change will not become a big surprise for someone when it start to work :) (In reply to fodillemlinkarim from comment #3) Do you have a full name you'd like me to use as the author of the git commit for this fix? So far what I would use is "Karim <fodillemlinkarim@gmail.com>" (In reply to John Baldwin from comment #5) Please use Karim Fodil-Lemelin Thanks (In reply to John Baldwin from comment #5) Since this was work founded by my employer XipLink Inc. might as well use my work email: kfl@xiplink.com Thanks. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=62b1faa3b7495de22a3225e42dabe6ce8c371e86 commit 62b1faa3b7495de22a3225e42dabe6ce8c371e86 Author: Karim Fodil-Lemelin <kfl@xiplink.com> AuthorDate: 2024-02-16 01:57:51 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2024-02-16 01:57:51 +0000 ipfw: Skip to the start of the loop when following a keep-state rule When a packet matches an existing dynamic rule for a keep-state rule, the matching engine advances the "instruction pointer" to the action portion of the rule skipping over the match conditions. However, the code was merely breaking out of the switch statement rather than doing a continue, so the remainder of the loop body after the switch was still executed. If the first action opcode contains an F_NOT but not an F_OR (such as an "untag" action), then match is toggled to 0, and the code exits the inner loop via a break which aborts processing of the actions. To fix, just use a continue instead of a break. PR: 276732 Reviewed by: jhb, ae MFC after: 2 weeks sys/netpfil/ipfw/ip_fw2.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d2c8cb41d1a2e531737a6da4cb9958bc497477c3 commit d2c8cb41d1a2e531737a6da4cb9958bc497477c3 Author: Karim Fodil-Lemelin <kfl@xiplink.com> AuthorDate: 2024-02-16 01:57:51 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2024-04-08 17:57:53 +0000 ipfw: Skip to the start of the loop when following a keep-state rule When a packet matches an existing dynamic rule for a keep-state rule, the matching engine advances the "instruction pointer" to the action portion of the rule skipping over the match conditions. However, the code was merely breaking out of the switch statement rather than doing a continue, so the remainder of the loop body after the switch was still executed. If the first action opcode contains an F_NOT but not an F_OR (such as an "untag" action), then match is toggled to 0, and the code exits the inner loop via a break which aborts processing of the actions. To fix, just use a continue instead of a break. PR: 276732 Reviewed by: jhb, ae MFC after: 2 weeks (cherry picked from commit 62b1faa3b7495de22a3225e42dabe6ce8c371e86) sys/netpfil/ipfw/ip_fw2.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3798c6487a21454020493517b613cda9a1753faf commit 3798c6487a21454020493517b613cda9a1753faf Author: Karim Fodil-Lemelin <kfl@xiplink.com> AuthorDate: 2024-02-16 01:57:51 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2024-04-08 17:57:57 +0000 ipfw: Skip to the start of the loop when following a keep-state rule When a packet matches an existing dynamic rule for a keep-state rule, the matching engine advances the "instruction pointer" to the action portion of the rule skipping over the match conditions. However, the code was merely breaking out of the switch statement rather than doing a continue, so the remainder of the loop body after the switch was still executed. If the first action opcode contains an F_NOT but not an F_OR (such as an "untag" action), then match is toggled to 0, and the code exits the inner loop via a break which aborts processing of the actions. To fix, just use a continue instead of a break. PR: 276732 Reviewed by: jhb, ae MFC after: 2 weeks (cherry picked from commit 62b1faa3b7495de22a3225e42dabe6ce8c371e86) sys/netpfil/ipfw/ip_fw2.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) |