[PATCH net-next v5 0/5] net/sched: act_pedit: minor improvements

[PATCH net-next v5 0/5] net/sched: act_pedit: minor improvements

[Pedro Tammela]

This series aims to improve the code and usability of act_pedit for
netlink users.

Patches 1-2 improves error reporting for extended keys parsing with extack.

Patch 3 checks the static offsets a priori on create/update. Currently,
this is done at the datapath for both static and runtime offsets.

Patch 4 removes a check from the datapath which is redundant since the
netlink parsing validates the key types.

Patch 5 changes the 'pr_info()' calls in the datapath to rate limited
versions.

v4->v5: Address suggestions by Jakub.
v3->v4: Break the old patch 1 into two patches.
v2->v3: Propagate nl_parse errors in patch 1 like the original version.
v1->v2: Added patch 3 to the series as discussed with Simon.

Pedro Tammela (5):
  net/sched: act_pedit: use NLA_POLICY for parsing 'ex' keys
  net/sched: act_pedit: use extack in 'ex' parsing errors
  net/sched: act_pedit: check static offsets a priori
  net/sched: act_pedit: remove extra check for key type
  net/sched: act_pedit: rate limit datapath messages

 net/sched/act_pedit.c | 85 ++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH net-next v5 1/5] net/sched: act_pedit: use NLA_POLICY for parsing 'ex' keys

[Pedro Tammela]

Transform two checks in the 'ex' key parsing into netlink policies
removing extra if checks.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 4559a1507ea5..45201f75e896 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -30,8 +30,9 @@ static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
 };
 
 static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = {
-	[TCA_PEDIT_KEY_EX_HTYPE]  = { .type = NLA_U16 },
-	[TCA_PEDIT_KEY_EX_CMD]	  = { .type = NLA_U16 },
+	[TCA_PEDIT_KEY_EX_HTYPE] =
+		NLA_POLICY_MAX(NLA_U16, TCA_PEDIT_HDR_TYPE_MAX),
+	[TCA_PEDIT_KEY_EX_CMD] = NLA_POLICY_MAX(NLA_U16, TCA_PEDIT_CMD_MAX),
 };
 
 static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
@@ -81,12 +82,6 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 		k->htype = nla_get_u16(tb[TCA_PEDIT_KEY_EX_HTYPE]);
 		k->cmd = nla_get_u16(tb[TCA_PEDIT_KEY_EX_CMD]);
 
-		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
-		    k->cmd > TCA_PEDIT_CMD_MAX) {
-			err = -EINVAL;
-			goto err_out;
-		}
-
 		k++;
 	}
 
-- 
2.34.1

[PATCH net-next v5 2/5] net/sched: act_pedit: use extack in 'ex' parsing errors

[Pedro Tammela]

We have extack available when parsing 'ex' keys, so pass it to
tcf_pedit_keys_ex_parse and add more detailed error messages.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 45201f75e896..24976cd4e4a2 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -36,7 +36,7 @@ static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = {
 };
 
 static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
-							u8 n)
+							u8 n, struct netlink_ext_ack *extack)
 {
 	struct tcf_pedit_key_ex *keys_ex;
 	struct tcf_pedit_key_ex *k;
@@ -57,12 +57,14 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
 
 		if (!n) {
+			NL_SET_ERR_MSG_MOD(extack, "Can't parse more extended keys than requested");
 			err = -EINVAL;
 			goto err_out;
 		}
 		n--;
 
 		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
+			NL_SET_ERR_MSG_ATTR(extack, ka, "Unknown attribute, expected extended key");
 			err = -EINVAL;
 			goto err_out;
 		}
@@ -73,8 +75,14 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 		if (err)
 			goto err_out;
 
-		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
-		    !tb[TCA_PEDIT_KEY_EX_CMD]) {
+		if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_HTYPE)) {
+			NL_SET_ERR_MSG(extack, "Missing required attribute");
+			err = -EINVAL;
+			goto err_out;
+		}
+
+		if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_CMD)) {
+			NL_SET_ERR_MSG(extack, "Missing required attribute");
 			err = -EINVAL;
 			goto err_out;
 		}
@@ -86,6 +94,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 	}
 
 	if (n) {
+		NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse");
 		err = -EINVAL;
 		goto err_out;
 	}
@@ -217,7 +226,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	}
 
 	nparms->tcfp_keys_ex =
-		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
+		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys, extack);
 	if (IS_ERR(nparms->tcfp_keys_ex)) {
 		ret = PTR_ERR(nparms->tcfp_keys_ex);
 		goto out_free;
-- 
2.34.1

[PATCH net-next v5 3/5] net/sched: act_pedit: check static offsets a priori

[Pedro Tammela]

Static key offsets should always be on 32 bit boundaries. Validate them on
create/update time for static offsets and move the datapath validation
for runtime offsets only.

iproute2 already errors out if a given offset and data size cannot be
packed to a 32 bit boundary. This change will make sure users which
create/update pedit instances directly via netlink also error out,
instead of finding out when packets are traversing.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 24976cd4e4a2..cc4dfb01c6c7 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -251,8 +251,16 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	memcpy(nparms->tcfp_keys, parm->keys, ksize);
 
 	for (i = 0; i < nparms->tcfp_nkeys; ++i) {
+		u32 offmask = nparms->tcfp_keys[i].offmask;
 		u32 cur = nparms->tcfp_keys[i].off;
 
+		/* The AT option can be added to static offsets in the datapath */
+		if (!offmask && cur % 4) {
+			NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 32bit boundaries");
+			ret = -EINVAL;
+			goto put_chain;
+		}
+
 		/* sanitize the shift value for any later use */
 		nparms->tcfp_keys[i].shift = min_t(size_t,
 						   BITS_PER_TYPE(int) - 1,
@@ -261,7 +269,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 		/* The AT option can read a single byte, we can bound the actual
 		 * value with uchar max.
 		 */
-		cur += (0xff & nparms->tcfp_keys[i].offmask) >> nparms->tcfp_keys[i].shift;
+		cur += (0xff & offmask) >> nparms->tcfp_keys[i].shift;
 
 		/* Each key touches 4 bytes starting from the computed offset */
 		nparms->tcfp_off_max_hint =
@@ -411,12 +419,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 					       sizeof(_d), &_d);
 			if (!d)
 				goto bad;
-			offset += (*d & tkey->offmask) >> tkey->shift;
-		}
 
-		if (offset % 4) {
-			pr_info("tc action pedit offset must be on 32 bit boundaries\n");
-			goto bad;
+			offset += (*d & tkey->offmask) >> tkey->shift;
+			if (offset % 4) {
+				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
+				goto bad;
+			}
 		}
 
 		if (!offset_valid(skb, hoffset + offset)) {
-- 
2.34.1

[PATCH net-next v5 4/5] net/sched: act_pedit: remove extra check for key type

[Pedro Tammela]

The netlink parsing already validates the key 'htype'.
Remove the datapath check as it's redundant.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index cc4dfb01c6c7..2fec4473d800 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -325,37 +325,28 @@ static bool offset_valid(struct sk_buff *skb, int offset)
 	return true;
 }
 
-static int pedit_skb_hdr_offset(struct sk_buff *skb,
-				enum pedit_header_type htype, int *hoffset)
+static void 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:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
-		if (skb_transport_header_was_set(skb)) {
+		if (skb_transport_header_was_set(skb))
 			*hoffset = skb_transport_offset(skb);
-			ret = 0;
-		}
 		break;
 	default:
-		ret = -EINVAL;
 		break;
 	}
-
-	return ret;
 }
 
 TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
@@ -388,10 +379,9 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 
 	for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
 		int offset = tkey->off;
+		int hoffset = 0;
 		u32 *ptr, hdata;
-		int hoffset;
 		u32 val;
-		int rc;
 
 		if (tkey_ex) {
 			htype = tkey_ex->htype;
@@ -400,12 +390,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			tkey_ex++;
 		}
 
-		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
-		if (rc) {
-			pr_info("tc action pedit bad header type specified (0x%x)\n",
-				htype);
-			goto bad;
-		}
+		pedit_skb_hdr_offset(skb, htype, &hoffset);
 
 		if (tkey->offmask) {
 			u8 *d, _d;
-- 
2.34.1

[PATCH net-next v5 5/5] net/sched: act_pedit: rate limit datapath messages

[Pedro Tammela]

Unbounded info messages in the pedit datapath can flood the printk
ring buffer quite easily depending on the action created.
As these messages are informational, usually printing some, not all,
is enough to bring attention to the real issue.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 2fec4473d800..fb93d4c1faca 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -396,8 +396,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			u8 *d, _d;
 
 			if (!offset_valid(skb, hoffset + tkey->at)) {
-				pr_info("tc action pedit 'at' offset %d out of bounds\n",
-					hoffset + tkey->at);
+				pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n",
+						    hoffset + tkey->at);
 				goto bad;
 			}
 			d = skb_header_pointer(skb, hoffset + tkey->at,
@@ -407,14 +407,13 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 
 			offset += (*d & tkey->offmask) >> tkey->shift;
 			if (offset % 4) {
-				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
+				pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n");
 				goto bad;
 			}
 		}
 
 		if (!offset_valid(skb, hoffset + offset)) {
-			pr_info("tc action pedit offset %d out of bounds\n",
-				hoffset + offset);
+			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
 			goto bad;
 		}
 
@@ -431,8 +430,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			val = (*ptr + tkey->val) & ~tkey->mask;
 			break;
 		default:
-			pr_info("tc action pedit bad command (%d)\n",
-				cmd);
+			pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd);
 			goto bad;
 		}
 
-- 
2.34.1

Re: [PATCH net-next v5 1/5] net/sched: act_pedit: use NLA_POLICY for parsing 'ex' keys

[Simon Horman]

On Fri, Apr 21, 2023 at 06:25:13PM -0300, Pedro Tammela wrote:
> Transform two checks in the 'ex' key parsing into netlink policies
> removing extra if checks.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v5 0/5] net/sched: act_pedit: minor improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 21 Apr 2023 18:25:12 -0300 you wrote:
> This series aims to improve the code and usability of act_pedit for
> netlink users.
> 
> Patches 1-2 improves error reporting for extended keys parsing with extack.
> 
> Patch 3 checks the static offsets a priori on create/update. Currently,
> this is done at the datapath for both static and runtime offsets.
> 
> [...]

Here is the summary with links:
  - [net-next,v5,1/5] net/sched: act_pedit: use NLA_POLICY for parsing 'ex' keys
    https://git.kernel.org/netdev/net-next/c/5036034572b7
  - [net-next,v5,2/5] net/sched: act_pedit: use extack in 'ex' parsing errors
    https://git.kernel.org/netdev/net-next/c/0c83c5210e18
  - [net-next,v5,3/5] net/sched: act_pedit: check static offsets a priori
    https://git.kernel.org/netdev/net-next/c/e1201bc781c2
  - [net-next,v5,4/5] net/sched: act_pedit: remove extra check for key type
    https://git.kernel.org/netdev/net-next/c/577140180ba2
  - [net-next,v5,5/5] net/sched: act_pedit: rate limit datapath messages
    https://git.kernel.org/netdev/net-next/c/e3c9673e2f6e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v5 3/5] net/sched: act_pedit: check static offsets a priori

[Ido Schimmel]

On Fri, Apr 21, 2023 at 06:25:15PM -0300, Pedro Tammela wrote:
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 24976cd4e4a2..cc4dfb01c6c7 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -251,8 +251,16 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  	memcpy(nparms->tcfp_keys, parm->keys, ksize);
>  
>  	for (i = 0; i < nparms->tcfp_nkeys; ++i) {
> +		u32 offmask = nparms->tcfp_keys[i].offmask;
>  		u32 cur = nparms->tcfp_keys[i].off;
>  
> +		/* The AT option can be added to static offsets in the datapath */
> +		if (!offmask && cur % 4) {
> +			NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 32bit boundaries");
> +			ret = -EINVAL;
> +			goto put_chain;

I think this leaks 'nparms->tcfp_keys'. See full syzkaller report here
[1].

> +		}
> +
>  		/* sanitize the shift value for any later use */
>  		nparms->tcfp_keys[i].shift = min_t(size_t,
>  						   BITS_PER_TYPE(int) - 1,

[1]
Syzkaller hit 'memory leak in tcf_pedit_init' bug.

BUG: memory leak
unreferenced object 0xffff88803d45e400 (size 1024):
  comm "syz-executor292", pid 563, jiffies 4295025223 (age 51.781s)
  hex dump (first 32 bytes):
    28 bd 70 00 fb db df 25 02 00 14 1f ff 02 00 02  (.p....%........
    00 32 00 00 1f 00 00 00 ac 14 14 3e 08 00 07 00  .2.........>....
  backtrace:
    [<ffffffff81bd0f2c>] kmemleak_alloc_recursive include/linux/kmemleak.h:42 [inline]
    [<ffffffff81bd0f2c>] slab_post_alloc_hook mm/slab.h:772 [inline]
    [<ffffffff81bd0f2c>] slab_alloc_node mm/slub.c:3452 [inline]
    [<ffffffff81bd0f2c>] __kmem_cache_alloc_node+0x25c/0x320 mm/slub.c:3491
    [<ffffffff81a865d9>] __do_kmalloc_node mm/slab_common.c:966 [inline]
    [<ffffffff81a865d9>] __kmalloc+0x59/0x1a0 mm/slab_common.c:980
    [<ffffffff83aa85c3>] kmalloc include/linux/slab.h:584 [inline]
    [<ffffffff83aa85c3>] tcf_pedit_init+0x793/0x1ae0 net/sched/act_pedit.c:245
    [<ffffffff83a90623>] tcf_action_init_1+0x453/0x6e0 net/sched/act_api.c:1394
    [<ffffffff83a90e58>] tcf_action_init+0x5a8/0x950 net/sched/act_api.c:1459
    [<ffffffff83a96258>] tcf_action_add+0x118/0x4e0 net/sched/act_api.c:1985
    [<ffffffff83a96997>] tc_ctl_action+0x377/0x490 net/sched/act_api.c:2044
    [<ffffffff83920a8d>] rtnetlink_rcv_msg+0x46d/0xd70 net/core/rtnetlink.c:6395
    [<ffffffff83b24305>] netlink_rcv_skb+0x185/0x490 net/netlink/af_netlink.c:2575
    [<ffffffff83901806>] rtnetlink_rcv+0x26/0x30 net/core/rtnetlink.c:6413
    [<ffffffff83b21cae>] netlink_unicast_kernel net/netlink/af_netlink.c:1339 [inline]
    [<ffffffff83b21cae>] netlink_unicast+0x5be/0x8a0 net/netlink/af_netlink.c:1365
    [<ffffffff83b2293f>] netlink_sendmsg+0x9af/0xed0 net/netlink/af_netlink.c:1942
    [<ffffffff8380c39f>] sock_sendmsg_nosec net/socket.c:724 [inline]
    [<ffffffff8380c39f>] sock_sendmsg net/socket.c:747 [inline]
    [<ffffffff8380c39f>] ____sys_sendmsg+0x3ef/0xaa0 net/socket.c:2503
    [<ffffffff838156d2>] ___sys_sendmsg+0x122/0x1c0 net/socket.c:2557
    [<ffffffff8381594f>] __sys_sendmsg+0x11f/0x200 net/socket.c:2586
    [<ffffffff83815ab0>] __do_sys_sendmsg net/socket.c:2595 [inline]
    [<ffffffff83815ab0>] __se_sys_sendmsg net/socket.c:2593 [inline]
    [<ffffffff83815ab0>] __x64_sys_sendmsg+0x80/0xc0 net/socket.c:2593



Syzkaller reproducer:
# {Threaded:false Repeat:true RepeatTimes:0 Procs:1 Slowdown:1 Sandbox: SandboxArg:0 Leak:true NetInjection:false NetDevices:false NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false IEEE802154:false Sysctl:false UseTmpDir:false HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false Fault:false FaultCall:0 FaultNth:0}}
r0 = socket$nl_route(0x10, 0x3, 0x0)
sendmsg$nl_route(r0, &(0x7f0000000200)={0x0, 0x0, &(0x7f00000001c0)={&(0x7f0000000100)=@ipv4_delroute={0x58, 0x19, 0x300, 0x70bd28, 0x25dfdbfb, {0x2, 0x0, 0x14, 0x1f, 0xff, 0x2, 0x0, 0x2, 0x3200}, [@RTA_PREFSRC={0x8, 0x7, @dev={0xac, 0x14, 0x14, 0x3e}}, @RTA_PREFSRC={0x8, 0x7, @initdev={0xac, 0x1e, 0x0, 0x0}}, @RTA_SPORT={0x6, 0x1c, 0x4e20}, @RTA_TABLE={0x8, 0xf, 0x368}, @RTA_PREFSRC={0x8, 0x7, @remote}, @RTA_TABLE={0x8, 0xf, 0xa2}, @RTA_METRICS={0x4}, @RTA_IIF={0x8}]}, 0x58}, 0x1, 0x0, 0x0, 0x4000040}, 0x80)
sendmsg$nl_route_sched(r0, &(0x7f0000000040)={0x0, 0x0, &(0x7f0000000080)={&(0x7f00000000c0)=ANY=[@ANYBLOB="601d0000300009000000000000000000000000004c1d0100481d01000a00010070656469740000001c1d0280c80e04"], 0x1d60}}, 0x0)


C reproducer:
// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE 

#include <dirent.h>
#include <endian.h>
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/prctl.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

static void sleep_ms(uint64_t ms)
{
	usleep(ms * 1000);
}

static uint64_t current_time_ms(void)
{
	struct timespec ts;
	if (clock_gettime(CLOCK_MONOTONIC, &ts))
	exit(1);
	return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
}

#define BITMASK(bf_off,bf_len) (((1ull << (bf_len)) - 1) << (bf_off))
#define STORE_BY_BITMASK(type,htobe,addr,val,bf_off,bf_len) *(type*)(addr) = htobe((htobe(*(type*)(addr)) & ~BITMASK((bf_off), (bf_len))) | (((type)(val) << (bf_off)) & BITMASK((bf_off), (bf_len))))

static bool write_file(const char* file, const char* what, ...)
{
	char buf[1024];
	va_list args;
	va_start(args, what);
	vsnprintf(buf, sizeof(buf), what, args);
	va_end(args);
	buf[sizeof(buf) - 1] = 0;
	int len = strlen(buf);
	int fd = open(file, O_WRONLY | O_CLOEXEC);
	if (fd == -1)
		return false;
	if (write(fd, buf, len) != len) {
		int err = errno;
		close(fd);
		errno = err;
		return false;
	}
	close(fd);
	return true;
}

static void kill_and_wait(int pid, int* status)
{
	kill(-pid, SIGKILL);
	kill(pid, SIGKILL);
	for (int i = 0; i < 100; i++) {
		if (waitpid(-1, status, WNOHANG | __WALL) == pid)
			return;
		usleep(1000);
	}
	DIR* dir = opendir("/sys/fs/fuse/connections");
	if (dir) {
		for (;;) {
			struct dirent* ent = readdir(dir);
			if (!ent)
				break;
			if (strcmp(ent->d_name, ".") == 0 || strcmp(ent->d_name, "..") == 0)
				continue;
			char abort[300];
			snprintf(abort, sizeof(abort), "/sys/fs/fuse/connections/%s/abort", ent->d_name);
			int fd = open(abort, O_WRONLY);
			if (fd == -1) {
				continue;
			}
			if (write(fd, abort, 1) < 0) {
			}
			close(fd);
		}
		closedir(dir);
	} else {
	}
	while (waitpid(-1, status, __WALL) != pid) {
	}
}

static void setup_test()
{
	prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
	setpgrp();
	write_file("/proc/self/oom_score_adj", "1000");
}

#define KMEMLEAK_FILE "/sys/kernel/debug/kmemleak"

static void setup_leak()
{
	if (!write_file(KMEMLEAK_FILE, "scan"))
	exit(1);
	sleep(5);
	if (!write_file(KMEMLEAK_FILE, "scan"))
	exit(1);
	if (!write_file(KMEMLEAK_FILE, "clear"))
	exit(1);
}

static void check_leaks(void)
{
	int fd = open(KMEMLEAK_FILE, O_RDWR);
	if (fd == -1)
	exit(1);
	uint64_t start = current_time_ms();
	if (write(fd, "scan", 4) != 4)
	exit(1);
	sleep(1);
	while (current_time_ms() - start < 4 * 1000)
		sleep(1);
	if (write(fd, "scan", 4) != 4)
	exit(1);
	static char buf[128 << 10];
	ssize_t n = read(fd, buf, sizeof(buf) - 1);
	if (n < 0)
	exit(1);
	int nleaks = 0;
	if (n != 0) {
		sleep(1);
		if (write(fd, "scan", 4) != 4)
	exit(1);
		if (lseek(fd, 0, SEEK_SET) < 0)
	exit(1);
		n = read(fd, buf, sizeof(buf) - 1);
		if (n < 0)
	exit(1);
		buf[n] = 0;
		char* pos = buf;
		char* end = buf + n;
		while (pos < end) {
			char* next = strstr(pos + 1, "unreferenced object");
			if (!next)
				next = end;
			char prev = *next;
			*next = 0;
			fprintf(stderr, "BUG: memory leak\n%s\n", pos);
			*next = prev;
			pos = next;
			nleaks++;
		}
	}
	if (write(fd, "clear", 5) != 5)
	exit(1);
	close(fd);
	if (nleaks)
		exit(1);
}

static void execute_one(void);

#define WAIT_FLAGS __WALL

static void loop(void)
{
	int iter = 0;
	for (;; iter++) {
		int pid = fork();
		if (pid < 0)
	exit(1);
		if (pid == 0) {
			setup_test();
			execute_one();
			exit(0);
		}
		int status = 0;
		uint64_t start = current_time_ms();
		for (;;) {
			if (waitpid(-1, &status, WNOHANG | WAIT_FLAGS) == pid)
				break;
			sleep_ms(1);
			if (current_time_ms() - start < 5000)
				continue;
			kill_and_wait(pid, &status);
			break;
		}
		check_leaks();
	}
}

uint64_t r[1] = {0xffffffffffffffff};

void execute_one(void)
{
		intptr_t res = 0;
	res = syscall(__NR_socket, 0x10ul, 3ul, 0);
	if (res != -1)
		r[0] = res;
*(uint64_t*)0x20000200 = 0;
*(uint32_t*)0x20000208 = 0;
*(uint64_t*)0x20000210 = 0x200001c0;
*(uint64_t*)0x200001c0 = 0x20000100;
*(uint32_t*)0x20000100 = 0x58;
*(uint16_t*)0x20000104 = 0x19;
*(uint16_t*)0x20000106 = 0x300;
*(uint32_t*)0x20000108 = 0x70bd28;
*(uint32_t*)0x2000010c = 0x25dfdbfb;
*(uint8_t*)0x20000110 = 2;
*(uint8_t*)0x20000111 = 0;
*(uint8_t*)0x20000112 = 0x14;
*(uint8_t*)0x20000113 = 0x1f;
*(uint8_t*)0x20000114 = -1;
*(uint8_t*)0x20000115 = 2;
*(uint8_t*)0x20000116 = 0;
*(uint8_t*)0x20000117 = 2;
*(uint32_t*)0x20000118 = 0x3200;
*(uint16_t*)0x2000011c = 8;
*(uint16_t*)0x2000011e = 7;
*(uint8_t*)0x20000120 = 0xac;
*(uint8_t*)0x20000121 = 0x14;
*(uint8_t*)0x20000122 = 0x14;
*(uint8_t*)0x20000123 = 0x3e;
*(uint16_t*)0x20000124 = 8;
*(uint16_t*)0x20000126 = 7;
*(uint8_t*)0x20000128 = 0xac;
*(uint8_t*)0x20000129 = 0x1e;
*(uint8_t*)0x2000012a = 0;
*(uint8_t*)0x2000012b = 1;
*(uint16_t*)0x2000012c = 6;
*(uint16_t*)0x2000012e = 0x1c;
*(uint16_t*)0x20000130 = htobe16(0x4e20);
*(uint16_t*)0x20000134 = 8;
*(uint16_t*)0x20000136 = 0xf;
*(uint32_t*)0x20000138 = 0x368;
*(uint16_t*)0x2000013c = 8;
*(uint16_t*)0x2000013e = 7;
*(uint8_t*)0x20000140 = 0xac;
*(uint8_t*)0x20000141 = 0x14;
*(uint8_t*)0x20000142 = 0x14;
*(uint8_t*)0x20000143 = 0xbb;
*(uint16_t*)0x20000144 = 8;
*(uint16_t*)0x20000146 = 0xf;
*(uint32_t*)0x20000148 = 0xa2;
*(uint16_t*)0x2000014c = 4;
STORE_BY_BITMASK(uint16_t, , 0x2000014e, 8, 0, 14);
STORE_BY_BITMASK(uint16_t, , 0x2000014f, 0, 6, 1);
STORE_BY_BITMASK(uint16_t, , 0x2000014f, 1, 7, 1);
*(uint16_t*)0x20000150 = 8;
*(uint16_t*)0x20000152 = 3;
*(uint32_t*)0x20000154 = 0;
*(uint64_t*)0x200001c8 = 0x58;
*(uint64_t*)0x20000218 = 1;
*(uint64_t*)0x20000220 = 0;
*(uint64_t*)0x20000228 = 0;
*(uint32_t*)0x20000230 = 0x4000040;
	syscall(__NR_sendmsg, r[0], 0x20000200ul, 0x80ul);
*(uint64_t*)0x20000040 = 0;
*(uint32_t*)0x20000048 = 0;
*(uint64_t*)0x20000050 = 0x20000080;
*(uint64_t*)0x20000080 = 0x200000c0;
memcpy((void*)0x200000c0, "\x60\x1d\x00\x00\x30\x00\x09\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x4c\x1d\x01\x00\x48\x1d\x01\x00\x0a\x00\x01\x00\x70\x65\x64\x69\x74\x00\x00\x00\x1c\x1d\x02\x80\xc8\x0e\x04", 47);
*(uint64_t*)0x20000088 = 0x1d60;
*(uint64_t*)0x20000058 = 1;
*(uint64_t*)0x20000060 = 0;
*(uint64_t*)0x20000068 = 0;
*(uint32_t*)0x20000070 = 0;
	syscall(__NR_sendmsg, r[0], 0x20000040ul, 0ul);

}
int main(void)
{
		syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
	syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
	syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
	setup_leak();
			loop();
	return 0;
}

Re: [PATCH net-next v5 3/5] net/sched: act_pedit: check static offsets a priori

[Pedro Tammela]

On 25/04/2023 09:13, Ido Schimmel wrote:
> On Fri, Apr 21, 2023 at 06:25:15PM -0300, Pedro Tammela wrote:
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index 24976cd4e4a2..cc4dfb01c6c7 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -251,8 +251,16 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>>   	memcpy(nparms->tcfp_keys, parm->keys, ksize);
>>   
>>   	for (i = 0; i < nparms->tcfp_nkeys; ++i) {
>> +		u32 offmask = nparms->tcfp_keys[i].offmask;
>>   		u32 cur = nparms->tcfp_keys[i].off;
>>   
>> +		/* The AT option can be added to static offsets in the datapath */
>> +		if (!offmask && cur % 4) {
>> +			NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 32bit boundaries");
>> +			ret = -EINVAL;
>> +			goto put_chain;
> 
> I think this leaks 'nparms->tcfp_keys'. See full syzkaller report here
> [1].
> 


Hi Ido,

Indeed! Thanks for the report.
Can you run the syzkaller corpus with the following patch?

---

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index fb93d4c1faca..fc945c7e4123 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -258,7 +258,7 @@ static int tcf_pedit_init(struct net *net, struct 
nlattr *nla,
                 if (!offmask && cur % 4) {
                         NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 
32bit boundaries");
                         ret = -EINVAL;
-                       goto put_chain;
+                       goto out_free_keys;
                 }

                 /* sanitize the shift value for any later use */
@@ -291,6 +291,8 @@ static int tcf_pedit_init(struct net *net, struct 
nlattr *nla,

         return ret;

+out_free_keys:
+       kfree(nparms->tcfp_keys);
  put_chain:
         if (goto_ch)
                 tcf_chain_put_by_act(goto_ch);

Re: [PATCH net-next v5 3/5] net/sched: act_pedit: check static offsets a priori

[Ido Schimmel]

On Tue, Apr 25, 2023 at 09:27:32AM -0300, Pedro Tammela wrote:
> Can you run the syzkaller corpus with the following patch?

The C reproducer allows anyone to test a fix without any special
dependencies. The reproducer triggers the issue without the fix and does
not trigger the issue with the fix.

Thanks