From nobody Fri Jan 26 19:32:39 2024 X-Original-To: freebsd-net@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TM7Bg5Vmgz58hhp; Fri, 26 Jan 2024 19:32:43 +0000 (UTC) (envelope-from kfodil-lemelin@xiplink.com) Received: from CAN01-YT3-obe.outbound.protection.outlook.com (mail-yt3can01on2130.outbound.protection.outlook.com [40.107.115.130]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mail.protection.outlook.com", Issuer "DigiCert Cloud Services CA-1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TM7Bf6w2yz4887; Fri, 26 Jan 2024 19:32:42 +0000 (UTC) (envelope-from kfodil-lemelin@xiplink.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=xiplink.com header.s=selector2 header.b=WGYZgMHx; arc=pass ("microsoft.com:s=arcselector9901:i=1"); dmarc=pass (policy=reject) header.from=xiplink.com; spf=pass (mx1.freebsd.org: domain of kfodil-lemelin@xiplink.com designates 40.107.115.130 as permitted sender) smtp.mailfrom=kfodil-lemelin@xiplink.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MyRh3AdGLr/jJAtzB+7p3tftv4eBCcagOE8QguoTpxRbR66zcYttU0iEAyjyPh6KW85ASj20Sa1AMcMFbzAd1lER/59xXlfn7V+1rWgelQJm2jGCqLDSRolr5swAWwn4sKWeQom75+j174Bqc6/QEn1/A+fWMIxdJrVBZxeB82QaI9kL7RnzsuI4f6labUAJFvM/ujvg3tbniWRB2Ac56DQnUdpI0lwea0DuNqr1bQBamGwmnkuIfl1+VpDkMK6XFL8mztjYt7Qvqv7jlxmHyQO7nAPtiLu1x5yJPKZBDqoxgX3b64lZtOb9JRl+uwdg84ZCscLqGJ7qAHNrYvXluQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=FVWLPLP8SfGdchqxEX/bJIOZWot80DUVp4Z6G38om7o=; b=M1184+V/ANg1f+3CLlel2W+IfpT76BZjc3uQVcYlZ/1uJV4Ie2UyiTajeRx9VtsCvEu26xJiiYGe6MBvpSsW4unr7IO3FiTadD5WYIuUvUXbgX+Iz8869RXLfk3Qb8bKAdh2KYDuG9O//KKIfF9qUdrHzJC6IrpP8zlIerhxiKkCcWvwg48KzFkO/9wXTdeN/8MLVU4+ulDVDd0rKj2CQTWLa4Izr7lfQ7Hm9WlTrW5JCvNnpEKspf/71uciAd2xjlhw2f3YQXY9Zok111PVDZRbiiOkMZL+GnLeSKG6VtrC/Rhy2W+sB6TbIi5p+ZDjlTaQO02acxSpeq2WHDtWcA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=xiplink.com; dmarc=pass action=none header.from=xiplink.com; dkim=pass header.d=xiplink.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=xiplink.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FVWLPLP8SfGdchqxEX/bJIOZWot80DUVp4Z6G38om7o=; b=WGYZgMHxIE6XT68PNNHiHGCVMWlnrPVam222bML7rg2OLnsJbBXedod0jBtirknSYG64YZtR9A39a91xmPoFfvU5gdtK1IHNN1SZWI3Ozg+p+AP1UMqbyR/t6W9zy3v+iGGwTOv4yP2/NLOvWntnpWDfKc+YZqRKYB3RZ1Y2+5jAiRlLIOpMmY/Haj/Kx09Z48cg18L2fYGub6uGauGqM6uEWUV3NL+qR8yJDM/rkCFyCX1dg66aydtvsMnO5B/9dVQB2Iiox+c1TT/qdYsuhxWTkpY9PamCgaewFtgzo+GeRI36iDpjRYDo+2LBc84gI0si6LstVRCB9TWnj7UF2Q== Received: from QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:3a::13) by YT1PR01MB9097.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:b01:cc::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7228.27; Fri, 26 Jan 2024 19:32:41 +0000 Received: from QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM ([fe80::fedb:1b11:3b0f:924a]) by QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM ([fe80::fedb:1b11:3b0f:924a%5]) with mapi id 15.20.7228.027; Fri, 26 Jan 2024 19:32:41 +0000 Content-Type: multipart/alternative; boundary="------------UN2Nz2O9jdpjqwIdAJgUl4uu" Message-ID: <427708c9-7d62-443a-ad0f-4848bf8aff8f@xiplink.com> Date: Fri, 26 Jan 2024 14:32:39 -0500 User-Agent: Mozilla Thunderbird Subject: Re: tag/untag Content-Language: en-US References: <7915c1d1-99a3-4459-85ac-9451bec06c24@xiplink.com> To: Mikhail Holt , freebsd-ipfw@FreeBSD.org, FreeBSD Net Cc: Kristof Provost , imp@freebsd.org From: Karim Fodil-Lemelin In-Reply-To: <7915c1d1-99a3-4459-85ac-9451bec06c24@xiplink.com> X-Forwarded-Message-Id: <7915c1d1-99a3-4459-85ac-9451bec06c24@xiplink.com> X-ClientProxiedBy: YQZPR01CA0075.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c01:84::10) To QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:3a::13) List-Id: Networking and TCP/IP with FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-net List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-net@freebsd.org MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: QB1PR01MB3428:EE_|YT1PR01MB9097:EE_ X-MS-Office365-Filtering-Correlation-Id: f328602f-3824-4ade-d5fa-08dc1ea58f7e X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: qM90gb7M4tbuxIzZ1eHbDRVNlgsTMpNaLZ7w/E+4FoD9z4+B7sPfkfk8x/oAjCyMSo/LQffGp2+MyiWkfkNMLUGBJNxs3BqTDGB36ysuqwLX5LU3yFm9tX6TXr21U2OHatVBYv4Jf5Gw+RsR7S2ym0OhhrQUbMAqOyPeZ8u8QcDBpVaF4uEqAbqlg7noyJIn4WtupeGo/nVksSs/ws+UTU4TrA843vG06KS6iNOuIYe3QtpFK+dartf2RW+StMGmZgneuZZ5WmQQrKuYvxTEkwxSL8174MfgFD5nRR+xdbQxzkwGVrlhgI7iRTQYc0d6j+iaRyFG5i8RLQ+pLH2OM2C5vBgwjFFe6fyprNsTPYoVbPmBigL+fownJ5O7faU9o+t8+cRpnOB+xHKBPKmRsuH9gvPE40mk6LKgZShkj22CE1mtOER0llUOzNxeac9rYGmRLSKiz6JoIB9ob8UhQTo1wVqTDPEmZGlztTrMyxRsxPf8epY+hY0GouGlSqH6fwgzs2KbRPQXaYcEDmXJcfxm0obDiLpCG98LrJovX4H0nEuTw7i1/ms3TVVWpgs5b+R6dO22LI2aQsskJFTKdleUfqnB3+NLPd9AAQSdWfqqkFiJjAg2u6ONOqVTcHDZnEMDq6MfgAYPUlVPhIqlzw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFS:(13230031)(366004)(346002)(39830400003)(396003)(136003)(376002)(230922051799003)(186009)(64100799003)(451199024)(1800799012)(83380400001)(3480700007)(6512007)(2616005)(38100700002)(8936002)(5660300002)(4326008)(110136005)(8676002)(66556008)(478600001)(6486002)(30864003)(6506007)(33964004)(66476007)(2906002)(66946007)(53546011)(316002)(41300700001)(31696002)(36756003)(86362001)(31686004)(43740500002)(45980500001);DIR:OUT;SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MXEweWhjWnI3ajZua3lyNXRaY1ZlS0w0UjFjcGZCc3d2aU52aWtXa0JFTmV2?= =?utf-8?B?VzVyTTVBbERzd3Bybmx1OHg3N2NGWmRFWGF2NVBOeGQvREpXTDErR2J0Ynp3?= =?utf-8?B?OHFIVEtFaTlsczFKUTVYYW8xRU1LSFhwakpHNzloMitjRVRxenFCWmptcllT?= =?utf-8?B?ZDk0cXJyd21vbmwydzI2RnBYcEFYaWtGVzJmamFKMG5mK3l3N2ZBUy9kRng4?= =?utf-8?B?OUhMOG56OWVKQVA3K0EyQk1IdHEyRjA1Um14TGdKNTA4WXI5NEpxQm96MjB1?= =?utf-8?B?YXBYKzB5UTlkcTNTeDhrK2I4T0haajc1ekkvR2VrWnB2TWluRnlPWWVOZHlp?= =?utf-8?B?aXdmM2xtRHVnYTgzNXJ0ZUhLWTVncU1nZ0tBazBZSUpKa01DT2dySW5ZVG9j?= =?utf-8?B?TVNlK3JmaUtlK2lRUitEL0RmaVVFV3J5U0lDSUxmUkt4ZHBvTFZuZTlLcHZC?= =?utf-8?B?QkJsZjF0U1NocTZvNXhjblA5VG1VZVE2dkxqaytWT3EzNkhMTlpxUHkyamJw?= =?utf-8?B?M05FU3BtOGtHU3A5VHRteDh2TXI5RHZoVHZBcWdHWnNDaFBRRUQ3VVFDbmxU?= =?utf-8?B?N3NqdWd5UFFtellrWlI4WW1WbkFWNlM5UWpacFlLNjJpaXo4MXNBU2xGQ3do?= =?utf-8?B?aHdjaW5mNHJ3MzVIb3BYczFNUWVDVUJXZWRtYnUwbENGVzJzQXo0THJWVmdR?= =?utf-8?B?b1h0WDA3VDZ1QitoRkh0cXJES0pqeFN2aVlFWWZhSTRqWWVvWTVsNVFaOFM4?= =?utf-8?B?YTdyTEJnOUlFY0NkN040NzZUS0twSFg0Y29GMDlYMUt4d0JleUlFbUs3MlN5?= =?utf-8?B?aVY3dFBRK05EZTA5U3ZpTGV3eDlSL0V1MStRcjJ6Q05MSGsyZENHZm44ZUxI?= =?utf-8?B?TGdaOHRGMUJ2QWp3NWRoTmxzZ0UxWm13NG9lZzRGazJSYjFNRkRRUWFKSU9w?= =?utf-8?B?V0hTL1V3cGNhbXFxWlRPdDI1QUpZRzF0N3NJRDdqQ0kxaTFldisxNFYwTzli?= =?utf-8?B?dWduUUNMV0tSVmRTeGkxL25FM01McUw3MElGaU1wakpqbERxSjBBa1pKR3pR?= =?utf-8?B?dkpYY2dKNTY2dHlIM1cra01MMllHczFXbFJlalJ0TGtEMVNZS1VsSENWNkkr?= =?utf-8?B?UkJNdFovZkRPWlFmaW1JMTNLUmtoRU85Y2ZodEplZFN4cDFiMzgwZlpRVVVV?= =?utf-8?B?UWNuVHZkWFl2U1BxMHczSDhOb0FrN1kvWk5Zb0pXQWdpMlpwaXFZNEpoRG5u?= =?utf-8?B?amg0T2Y5YzJ2VFFVcnFXRDQxVDFvdkJwUnZzM1Q1QUNoWmdKSXlaYjFadWg0?= =?utf-8?B?VEVvNGo2TjRIK3FIdXczNFFwUjA3RG5XNW82SktBOERpRnJKYWpsa0NPbzJy?= =?utf-8?B?R2lNOWVzM1ExSFlEOVBoRzJHK2hVRDBVL3JsYklydFlqdGxLdnNaQ01qTUlT?= =?utf-8?B?b1hGSUNzeVd1UVBEYUNNQVdQMjlsR05DNDRHSTJadEppKzhPbkh2YlpITmZ3?= =?utf-8?B?YnVoWUF1QnRCZEpuMmhlVHQ1dEFzVkNPRzh3Y044U0Z0aHUvSHlhU1czUXRM?= =?utf-8?B?RGgxZTBPU0grbldsT3FhRnpNVU1GbHFzak9MVStpR0RPRTR2eG4wVVQxbmEy?= =?utf-8?B?cGNsdC8vNC9kWmRHUW1kVjBXUU9zMnpMWTJMUURKTFJRSjEwYUhKWnNnd296?= =?utf-8?B?Y3paRzZFV2NmdWMzeUd5dUM5YmpKV2ExOGZ4c0VMbDkvbzkyTCtYc085K1Vm?= =?utf-8?B?dDRGN3ZZZDVsM0NSd2RmWVFUMnEzOHpyQVJvc1dYVWZyTFErRTBDV1hHOG9T?= =?utf-8?B?UlkwaFowM01yeDZqYThGeFE3ckI1OGt3L0p1SGxqbjZUaHlvbjV1UFlsOGxE?= =?utf-8?B?QXhKUEJXMHgwS0RhVStyQkY5YnFEMzJid0xJTFlhM1BDRU1ncEZGZHJ2OUhU?= =?utf-8?B?cnFyK0ZZUndqZjZ0d1lsMVZtckNJOTh2YlFsVzlZNkxxdE10VktabUJpYnd6?= =?utf-8?B?YkZiaVMwc2NQM1FUTEwvb292a3V0eEczQzRYdVBkcWU2dHNwOTBLdWs3TUIv?= =?utf-8?B?Ym8rMnJMZW9CdXJZR3c4RXR2UHp3dmtLYjdhQ1JHN084V1hKOG5Ga0cwL1Nm?= =?utf-8?B?SWZjVVNud0NLTy9mZXowSjg2QURhQm1VcVU3SkQ2N0lMcUdlbXY3VFpxTS9j?= =?utf-8?B?ZHpVWEthQ1dwQzFVb1NidEdiR3Q4SUE1VlhFUTBqT3MvcndnRzVuTjZZbDlB?= =?utf-8?B?cUxvU3VPTU9ZNXhiRi8zYndldmRBPT0=?= X-OriginatorOrg: xiplink.com X-MS-Exchange-CrossTenant-Network-Message-Id: f328602f-3824-4ade-d5fa-08dc1ea58f7e X-MS-Exchange-CrossTenant-AuthSource: QB1PR01MB3428.CANPRD01.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 19:32:41.0974 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 14f927ba-c95b-4aa6-b674-375045ee9d4d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: lceuzQHP3MfVEe9m16dfRdFJ4qwMEWdyIZL4a8iPBpKdFmtTUSLeEsU8y14SKQR8ADO1UR92K/hy9yXrEi25NuBhz3/080o7RAhViDyAk/w= X-MS-Exchange-Transport-CrossTenantHeadersStamped: YT1PR01MB9097 X-Spamd-Bar: --- X-Spamd-Result: default: False [-3.46 / 15.00]; SUSPICIOUS_RECIPS(1.50)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; ARC_ALLOW(-1.00)[microsoft.com:s=arcselector9901:i=1]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.97)[-0.975]; DMARC_POLICY_ALLOW(-0.50)[xiplink.com,reject]; R_SPF_ALLOW(-0.20)[+ip4:40.107.0.0/16:c]; R_DKIM_ALLOW(-0.20)[xiplink.com:s=selector2]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; XM_UA_NO_VERSION(0.01)[]; TO_DN_SOME(0.00)[]; FREEMAIL_TO(0.00)[gmail.com,FreeBSD.org,freebsd.org]; DKIM_TRACE(0.00)[xiplink.com:+]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RCVD_TLS_LAST(0.00)[]; FROM_HAS_DN(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[40.107.115.130:from]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; FROM_EQ_ENVFROM(0.00)[]; ASN(0.00)[asn:8075, ipnet:40.104.0.0/14, country:US]; MLMMJ_DEST(0.00)[freebsd-ipfw@FreeBSD.org,freebsd-net@freebsd.org]; TAGGED_RCPT(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; RWL_MAILSPIKE_POSSIBLE(0.00)[40.107.115.130:from]; RCPT_COUNT_FIVE(0.00)[5] X-Rspamd-Queue-Id: 4TM7Bf6w2yz4887 --------------UN2Nz2O9jdpjqwIdAJgUl4uu Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Mikhail, This is a very old bug in FreeBSD/ipfw O_PROBE_STATE and the good news is I have a fix for it. The fix is very easy and I will try to explain here what the bug is and what the fix is. The issue is only related to rules that have the F_NOT bit set and that are the beginning of the actions list of a keep-state rule. I'm also CCing some other folks that have authority in moving this forward so it hopefully gets into main eventually. If you are like me, this is a perfect read for a Friday afternoon ;) So here is how it goes: When you create a keep-state rule, the userland part of ipfw will insert a O_PROBE_STATE action at the very beginning of the rule so when a packet hits the rule the kernel will PROBE the list of installed dynamic rules for a match before installing such a rule (we don't want to create more than one dynamic rule per parent rule). The code responsible for this looks like this in ipfw2.c:         /*          * generate O_PROBE_STATE if necessary          */         if (have_state && have_state->opcode != O_CHECK_STATE && !have_rstate) {                 fill_cmd(dst, O_PROBE_STATE, 0, have_state->arg1);                 dst = next_cmd(dst, &rblen);         } Now ipfw userland also does something else when building the list of actions for a rule, it records the actions offset in the rule and put those actions together in a predetermined way, here is the section of ipfw2.c that does something like this:         /*          * start action section          */         rule->act_ofs = dst - rule->cmd;         /* put back O_LOG, O_ALTQ, O_TAG if necessary */ log --> if (have_log) {                 i = F_LEN(have_log);                 CHECK_RBUFLEN(i);                 bcopy(have_log, dst, i * sizeof(uint32_t));                 dst += i;         }         if (have_altq) {                 i = F_LEN(have_altq);                 CHECK_RBUFLEN(i);                 bcopy(have_altq, dst, i * sizeof(uint32_t));                 dst += i;         } tag --> if (have_tag) {                 i = F_LEN(have_tag);                 CHECK_RBUFLEN(i);                 bcopy(have_tag, dst, i * sizeof(uint32_t));                 dst += i;         } Nothing wrong with all this and if you are still with me (kudos to you) please take a moment to notice the 'log' action is _before_ the 'tag' action. This will be important later to understand why some rules in your example works and why some don't, although you may already have a clue... Now let's transport ourselves into the kernel code, precisely in sys/netpfil/ipfw/ip_fw2.c around line 2865. Here we are inside the O_PROBE_STATE case and notice how after we find a dynamic entry how the kernel will 'reset' the cmd pointer to the action part of the parent rule by doing something like this: /*                                          * Found dynamic entry, jump to the                                          * 'action' part of the parent rule                                          * by setting f, cmd, l and clearing                                          * cmdlen.                                          */                                         f = q;                                         f_pos = dyn_info.f_pos; cmd pointer is reset -->                cmd = ACTION_PTR(f);                                         l = f->cmd_len - f->act_ofs;                                         cmdlen = 0;                                         match = 1;                                         break;                                 } At this precise moment (pointed by the -->), if we refer to your example below, we would be hitting the dynamic rule entry and have cmd pointer reset to the ACTION part of the 'untag 10' action you have entered. We then set a few things and break from the switch statement. Now this gets us at the end of the switch statement where FreeBSD does something like this (around line 3337): /*                          * if we get here with l=0, then match is irrelevant.                          */ cmd is 'untag' -->      if (cmd->len & F_NOT)                                 match = !match;                         if (match) {                                 if (cmd->len & F_OR)                                         skip_or = 1;                         } else {                                 if (!(cmd->len & F_OR)) /* not an OR block, */ exits too early -->                     break;          /* try next rule    */                         }                 }       /* end of inner loop, scan opcodes */ If you look at the check for F_NOT bit (which is cleverly folded in the len part of the cmd) you realize that check is actually made against the 'untag' action and since untag is actually NOT tag it will match and change match into !match. The code will not try to go through the actual action and it will exit the loop too early. Essentially, by resetting the cmd action pointer we should give a chance for O_TAG (F_NOT) to be called and not simply turn match into a no match. The fix for this is very simple and goes like this: diff --git a/sys/netpfil/ipfw/ip_fw2.c b/sys/netpfil/ipfw/ip_fw2.c index d2b01fd..57c02dc 100644 --- a/sys/netpfil/ipfw/ip_fw2.c +++ b/sys/netpfil/ipfw/ip_fw2.c @@ -2887,7 +2887,8 @@ do { \                                   l = f->cmd_len - f->act_ofs;                                   cmdlen = 0;                                   match = 1; -                                 break; +                                 continue; +                                 break; /* not reached */                                 }                                 /*                                  * Dynamic entry not found. If CHECK_STATE, Now why did it work for you when you used log? Well I'm sure you remember that, because O_LOG is inserted _before_ O_TAG and that O_LOG doesn't have the F_NOT bit set then match is still 1 at the end of the loop and the actual cmd (LOG in this case) will have a chance to execute. Finally, if your still reading, I think this bug has been in ipfw for a very long time, great catch buddy! Best, Karim. PS: If you feel like compiling a kernel with my fix and testing it I'm sure the community would appreciate your feedback. On 2023-11-08 2:26 a.m., Mikhail Holt wrote: > Hello List, > > On a recent Stable 13 test host I, by accident, found that: > > /sbin/ipfw -q add 0031 allow              tcp from 192.168.64.0/24 to > me dst-port ssh in via igb3 setup keep-state   WORKS > > /sbin/ipfw -q add 0031 allow log          tcp from 192.168.64.0/24 to > me dst-port ssh in via igb3 setup keep-state   WORKS > > /sbin/ipfw -q add 0031 allow log tag   10 tcp from 192.168.64.0/24 to > me dst-port ssh in via igb3 setup keep-state   WORKS > > /sbin/ipfw -q add 0031 allow log untag 10 tcp from 192.168.64.0/24 to > me dst-port ssh in via igb3 setup keep-state   WORKS > > /sbin/ipfw -q add 0031 allow     untag 10 tcp from 192.168.64.0/24 to > me dst-port ssh in via igb3 setup keep-state   DOES NOT WORK? > - A dynamic rule is created as per the rules that work. > - Packets are logged by a deny all rule which of course is never > reached by the rules that work. > > Not a real issue for me but thought it worth noting. > > Mik. --------------UN2Nz2O9jdpjqwIdAJgUl4uu Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi Mikhail,

This is a very old bug in FreeBSD/ipfw O_PROBE_STATE and the good news is I have a fix for it. The fix is very easy and I will try to explain here what the bug is and what the fix is. The issue is only related to rules that have the F_NOT bit set and that are the beginning of the actions list of a keep-state rule. I'm also CCing some other folks that have authority in moving this forward so it hopefully gets into main eventually.

If you are like me, this is a perfect read for a Friday afternoon ;)

So here is how it goes:

When you create a keep-state rule, the userland part of ipfw will insert a O_PROBE_STATE action at the very beginning of the rule so when a packet hits the rule the kernel will PROBE the list of installed dynamic rules for a match before installing such a rule (we don't want to create more than one dynamic rule per parent rule). The code responsible for this looks like this in ipfw2.c:

        /*
         * generate O_PROBE_STATE if necessary
         */
        if (have_state && have_state->opcode != O_CHECK_STATE && !have_rstate) {
                fill_cmd(dst, O_PROBE_STATE, 0, have_state->arg1);
                dst = next_cmd(dst, &rblen);
        }  

Now ipfw userland also does something else when building the list of actions for a rule, it records the actions offset in the rule and put those actions together in a predetermined way, here is the section of ipfw2.c that does something like this:

        /*
         * start action section
         */
        rule->act_ofs = dst - rule->cmd;
           
        /* put back O_LOG, O_ALTQ, O_TAG if necessary */
log --> if (have_log) {
                i = F_LEN(have_log);
                CHECK_RBUFLEN(i);
                bcopy(have_log, dst, i * sizeof(uint32_t));
                dst += i;
        }
        if (have_altq) {
                i = F_LEN(have_altq);
                CHECK_RBUFLEN(i);
                bcopy(have_altq, dst, i * sizeof(uint32_t));
                dst += i;
        }
tag --> if (have_tag) {
                i = F_LEN(have_tag);
                CHECK_RBUFLEN(i);
                bcopy(have_tag, dst, i * sizeof(uint32_t));
                dst += i;   
        }

Nothing wrong with all this and if you are still with me (kudos to you) please take a moment to notice the 'log' action is _before_ the 'tag' action. This will be important later to understand why some rules in your example works and why some don't, although you may already have a clue...

Now let's transport ourselves into the kernel code, precisely in sys/netpfil/ipfw/ip_fw2.c around line 2865. Here we are inside the O_PROBE_STATE case and notice how after we find a dynamic entry how the kernel will 'reset' the cmd pointer to the action part of the parent rule by doing something like this:

                                       /*
                                         * Found dynamic entry, jump to the
                                         * 'action' part of the parent rule
                                         * by setting f, cmd, l and clearing
                                         * cmdlen.
                                         */
                                        f = q;
                                        f_pos = dyn_info.f_pos;
cmd pointer is reset -->                cmd = ACTION_PTR(f);
                                        l = f->cmd_len - f->act_ofs;
                                        cmdlen = 0;
                                        match = 1;
                                        break;
                                }

At this precise moment (pointed by the -->), if we refer to your example below, we would be hitting the dynamic rule entry and have cmd pointer reset to the ACTION part of the 'untag 10' action you have entered. We then set a few things and break from the switch statement. Now this gets us at the end of the switch statement where FreeBSD does something like this (around line 3337):

                        /*
                         * if we get here with l=0, then match is irrelevant.
                         */

cmd is 'untag' -->      if (cmd->len & F_NOT)
                                match = !match;

                        if (match) {
                                if (cmd->len & F_OR)
                                        skip_or = 1;
                        } else {
                                if (!(cmd->len & F_OR)) /* not an OR block, */
exits too early -->                     break;          /* try next rule    */
                        }

                }       /* end of inner loop, scan opcodes */

If you look at the check for F_NOT bit (which is cleverly folded in the len part of the cmd) you realize that check is actually made against the 'untag' action and since untag is actually NOT tag it will match and change match into !match. The code will not try to go through the actual action and it will exit the loop too early.

Essentially, by resetting the cmd action pointer we should give a chance for O_TAG (F_NOT) to be called and not simply turn match into a no match. The fix for this is very simple and goes like this:

diff --git a/sys/netpfil/ipfw/ip_fw2.c b/sys/netpfil/ipfw/ip_fw2.c
index d2b01fd..57c02dc 100644
--- a/sys/netpfil/ipfw/ip_fw2.c
+++ b/sys/netpfil/ipfw/ip_fw2.c
@@ -2887,7 +2887,8 @@ do {                                                              \
                                  l = f->cmd_len - f->act_ofs;
                                  cmdlen = 0;
                                  match = 1;
-                                 break;
+                                 continue;
+                                 break; /* not reached */
                                }
                                /*
                                 * Dynamic entry not found. If CHECK_STATE,


Now why did it work for you when you used log? Well I'm sure you remember that, because O_LOG is inserted _before_ O_TAG and that O_LOG doesn't have the F_NOT bit set then match is still 1 at the end of the loop and the actual cmd (LOG in this case) will have a chance to execute.

Finally, if your still reading, I think this bug has been in ipfw for a very long time, great catch buddy!

Best,

Karim.

PS: If you feel like compiling a kernel with my fix and testing it I'm sure the community would appreciate your feedback.

On 2023-11-08 2:26 a.m., Mikhail Holt wrote:
Hello List,

On a recent Stable 13 test host I, by accident, found that:

/sbin/ipfw -q add 0031 allow              tcp from 192.168.64.0/24 to me dst-port ssh in via igb3 setup keep-state   WORKS

/sbin/ipfw -q add 0031 allow log          tcp from 192.168.64.0/24 to me dst-port ssh in via igb3 setup keep-state   WORKS

/sbin/ipfw -q add 0031 allow log tag   10 tcp from 192.168.64.0/24 to me dst-port ssh in via igb3 setup keep-state   WORKS

/sbin/ipfw -q add 0031 allow log untag 10 tcp from 192.168.64.0/24 to me dst-port ssh in via igb3 setup keep-state   WORKS

/sbin/ipfw -q add 0031 allow     untag 10 tcp from 192.168.64.0/24 to me dst-port ssh in via igb3 setup keep-state   DOES NOT WORK?
- A dynamic rule is created as per the rules that work.
- Packets are logged by a deny all rule which of course is never reached by the rules that work.

Not a real issue for me but thought it worth noting.

Mik.

--------------UN2Nz2O9jdpjqwIdAJgUl4uu--