| 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
| ||
|
Message-ID: <b693fce4-7a05-ce88-ebe0-27b6fa03ed2f@mojatatu.com>
Date: Fri, 26 May 2023 11:03:16 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Max Tottenham <mtottenh@...mai.com>, netdev@...r.kernel.org
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang
<xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
Amir Vadai <amir@...ai.me>, Josh Hunt <johunt@...mai.com>,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2] net/sched: act_pedit: Parse L3 Header for L4 offset
On 26/05/2023 10:47, Pedro Tammela wrote:
> On 26/05/2023 06:58, Max Tottenham wrote:
>> Instead of relying on skb->transport_header being set correctly, opt
>> instead to parse the L3 header length out of the L3 headers for both
>> IPv4/IPv6 when the Extended Layer Op for tcp/udp is used. This fixes a
>> bug if GRO is disabled, when GRO is disabled skb->transport_header is
>> set by __netif_receive_skb_core() to point to the L3 header, it's later
>> fixed by the upper protocol layers, but act_pedit will receive the SKB
>> before the fixups are completed. The existing behavior causes the
>> following to edit the L3 header if GRO is disabled instead of the UDP
>> header:
>>
>> tc filter add dev eth0 ingress protocol ip flower ip_proto udp \
>> dst_ip 192.168.1.3 action pedit ex munge udp set dport 18053
>>
>> Also re-introduce a rate-limited warning if we were unable to extract
>> the header offset when using the 'ex' interface.
>>
>> Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to
>> the conventional network headers")
>
> Just a FYI: the automatic back port will probably fail because of a
> recent cleanup in this code
>
>> Signed-off-by: Max Tottenham <mtottenh@...mai.com>
>> Reviewed-by: Josh Hunt <johunt@...mai.com>
>> Reported-by: kernel test robot <lkp@...el.com>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/202305261541.N165u9TZ-lkp@intel.com/
>> ---
>> V1 -> V2:
>> * Fix minor bug reported by kernel test bot.
>>
>> ---
>> net/sched/act_pedit.c | 48 ++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 43 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index fc945c7e4123..d28335519459 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -13,7 +13,10 @@
>> #include <linux/rtnetlink.h>
>> #include <linux/module.h>
>> #include <linux/init.h>
>> +#include <linux/ip.h>
>> +#include <linux/ipv6.h>
>> #include <linux/slab.h>
>> +#include <net/ipv6.h>
>> #include <net/netlink.h>
>> #include <net/pkt_sched.h>
>> #include <linux/tc_act/tc_pedit.h>
>> @@ -327,28 +330,58 @@ static bool offset_valid(struct sk_buff *skb,
>> int offset)
>> return true;
>> }
>> -static void pedit_skb_hdr_offset(struct sk_buff *skb,
>> +static int pedit_l4_skb_offset(struct sk_buff *skb, int *hoffset,
>> const int header_type)
>> +{
>> + int noff = skb_network_offset(skb);
>> + struct iphdr *iph = NULL;
>> + int ret = -EINVAL;
>
> nit: Should be in reverse Christmas tree
>
>> +
>> + switch (skb->protocol) {
>> + case htons(ETH_P_IP):
>> + if (!pskb_may_pull(skb, sizeof(*iph) + noff))
>> + goto out;
>
> I might have missed something but is this really needed?
> https://elixir.bootlin.com/linux/latest/source/net/ipv4/ip_input.c#L456
Yes this obviously happens before the mentioned function.
Now I'm wondering if it's not better to use skb_header_pointer() instead...
>
>> + iph = ip_hdr(skb);
>> + *hoffset = noff + iph->ihl * > + ret = 0;
>> + break;
>> + case htons(ETH_P_IPV6):
>> + *hoffset = 0;
> nit: Not needed
>
>> + ret = ipv6_find_hdr(skb, hoffset, header_type, NULL, NULL) ==
>> header_type ? 0 : -EINVAL;
>> + break;
>> + }
>> +out:
>> + return ret;
>> +}
>> +
>> +static int pedit_skb_hdr_offset(struct sk_buff *skb,
>> enum pedit_header_type htype, int *hoffset)
>> {
>> + int ret = -EINVAL;
>> /* 'htype' is validated in the netlink parsing */
>> switch (htype) {
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
>> - if (skb_mac_header_was_set(skb))
>> + if (skb_mac_header_was_set(skb)) {
>> *hoffset = skb_mac_offset(skb);
>> + ret = 0;
>> + }
>> break;
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
>> *hoffset = skb_network_offset(skb);
>> + ret = 0;
>> break;
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
>> + ret = pedit_l4_skb_offset(skb, hoffset, IPPROTO_TCP);
>> + break;
>> case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
>> - if (skb_transport_header_was_set(skb))
>> - *hoffset = skb_transport_offset(skb);
>> + ret = pedit_l4_skb_offset(skb, hoffset, IPPROTO_UDP);
>> break;
>> default:
>> + ret = -EINVAL;
>
> nit: Not needed
>
>> break;
>> }
>> + return ret;
>> }
>> TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>> @@ -384,6 +417,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff
>> *skb,
>> int hoffset = 0;
>> u32 *ptr, hdata;
>> u32 val;
>> + int rc;
>> if (tkey_ex) {
>> htype = tkey_ex->htype;
>> @@ -392,7 +426,11 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct
>> sk_buff *skb,
>> tkey_ex++;
>> }
>> - pedit_skb_hdr_offset(skb, htype, &hoffset);
>> + rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
>> + if (rc) {
>> + pr_info_ratelimited("tc action pedit unable to extract
>> header offset for header type (0x%x)\n", htype);
>> + goto bad;
>> + }
>> if (tkey->offmask) {
>> u8 *d, _d;
>
Powered by blists - more mailing lists