diff mbox series

[net-next] netfilter: xt_SECMARK: add new revision to fix structure layout

Message ID 20210429133929.20161-1-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series [net-next] netfilter: xt_SECMARK: add new revision to fix structure layout | expand

Commit Message

Phil Sutter April 29, 2021, 1:39 p.m. UTC 
From: Pablo Neira Ayuso <pablo@netfilter.org>

This extension breaks when trying to delete rules, add a new revision to
fix this.

Fixes: 5e6874cdb8de ("[SECMARK]: Add xtables SECMARK target")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Formal submission of this patch which I received in private. Used it to
implement user space side and thereby discovered a bug:
secmark_tg_check_v1() would not populate the legacy data structure with
the 'secid' value initialized in the new data structure, thereby
breaking functionality for old user space.
---
 include/uapi/linux/netfilter/xt_SECMARK.h |  6 ++
 net/netfilter/xt_SECMARK.c                | 87 ++++++++++++++++++-----
 2 files changed, 74 insertions(+), 19 deletions(-)

Comments

Jan Engelhardt April 29, 2021, 3:11 p.m. UTC | #1 
On Thursday 2021-04-29 15:39, Phil Sutter wrote:
>
>This extension breaks when trying to delete rules, add a new revision to
>fix this.
>
>diff --git a/include/uapi/linux/netfilter/xt_SECMARK.h b/include/uapi/linux/netfilter/xt_SECMARK.h
>index 1f2a708413f5d..f412c87e675c1 100644
>--- a/include/uapi/linux/netfilter/xt_SECMARK.h
>+++ b/include/uapi/linux/netfilter/xt_SECMARK.h
>@@ -20,4 +20,10 @@ struct xt_secmark_target_info {
> 	char secctx[SECMARK_SECCTX_MAX];
> };
> 
>+struct xt_secmark_tginfo {
>+	__u8 mode;
>+	char secctx[SECMARK_SECCTX_MAX];
>+	__u32 secid;
>+};

that should be struct xt_secmark_tginfo_v1.

>+		.name		= "SECMARK",
>+		.revision	= 1,
>+		.family		= NFPROTO_UNSPEC,
>+		.checkentry	= secmark_tg_check_v2,

Can't have revision=1 and then call it _v2. That's just confusing.
Phil Sutter April 30, 2021, 10:22 a.m. UTC | #2 
Hi Jan,

On Thu, Apr 29, 2021 at 05:11:49PM +0200, Jan Engelhardt wrote:
[...]
> >+struct xt_secmark_tginfo {
> >+	__u8 mode;
> >+	char secctx[SECMARK_SECCTX_MAX];
> >+	__u32 secid;
> >+};
> 
> that should be struct xt_secmark_tginfo_v1.

The v0 struct is called xt_secmark_target_info, I guess Pablo tried to
shorten the name a bit. In conforming to "the standard", I'd then go
with xt_secmark_target_info_v1 instead. Fine with you?

> >+		.name		= "SECMARK",
> >+		.revision	= 1,
> >+		.family		= NFPROTO_UNSPEC,
> >+		.checkentry	= secmark_tg_check_v2,
> 
> Can't have revision=1 and then call it _v2. That's just confusing.

ACK, I missed that.

Thanks, Phil
Jan Engelhardt April 30, 2021, 10:23 a.m. UTC | #3 
On Friday 2021-04-30 12:22, Phil Sutter wrote:
>On Thu, Apr 29, 2021 at 05:11:49PM +0200, Jan Engelhardt wrote:
>[...]
>> >+struct xt_secmark_tginfo {
>> >+	__u8 mode;
>> >+	char secctx[SECMARK_SECCTX_MAX];
>> >+	__u32 secid;
>> >+};
>> 
>> that should be struct xt_secmark_tginfo_v1.
>
>The v0 struct is called xt_secmark_target_info, I guess Pablo tried to
>shorten the name a bit. In conforming to "the standard", I'd then go
>with xt_secmark_target_info_v1 instead. Fine with you?

That's ok.

>> >+		.checkentry	= secmark_tg_check_v2,
>> 
>> Can't have revision=1 and then call it _v2. That's just confusing.
>
>ACK, I missed that.
diff mbox series

Patch

diff --git a/include/uapi/linux/netfilter/xt_SECMARK.h b/include/uapi/linux/netfilter/xt_SECMARK.h
index 1f2a708413f5d..f412c87e675c1 100644
--- a/include/uapi/linux/netfilter/xt_SECMARK.h
+++ b/include/uapi/linux/netfilter/xt_SECMARK.h
@@ -20,4 +20,10 @@  struct xt_secmark_target_info {
 	char secctx[SECMARK_SECCTX_MAX];
 };
 
+struct xt_secmark_tginfo {
+	__u8 mode;
+	char secctx[SECMARK_SECCTX_MAX];
+	__u32 secid;
+};
+
 #endif /*_XT_SECMARK_H_target */
diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
index 75625d13e976c..1d28fa9104e30 100644
--- a/net/netfilter/xt_SECMARK.c
+++ b/net/netfilter/xt_SECMARK.c
@@ -24,10 +24,9 @@  MODULE_ALIAS("ip6t_SECMARK");
 static u8 mode;
 
 static unsigned int
-secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
+secmark_tg(struct sk_buff *skb, const struct xt_secmark_tginfo *info)
 {
 	u32 secmark = 0;
-	const struct xt_secmark_target_info *info = par->targinfo;
 
 	switch (mode) {
 	case SECMARK_MODE_SEL:
@@ -41,7 +40,7 @@  secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	return XT_CONTINUE;
 }
 
-static int checkentry_lsm(struct xt_secmark_target_info *info)
+static int checkentry_lsm(struct xt_secmark_tginfo *info)
 {
 	int err;
 
@@ -73,15 +72,14 @@  static int checkentry_lsm(struct xt_secmark_target_info *info)
 	return 0;
 }
 
-static int secmark_tg_check(const struct xt_tgchk_param *par)
+static int secmark_tg_check(const char *table, struct xt_secmark_tginfo *info)
 {
-	struct xt_secmark_target_info *info = par->targinfo;
 	int err;
 
-	if (strcmp(par->table, "mangle") != 0 &&
-	    strcmp(par->table, "security") != 0) {
+	if (strcmp(table, "mangle") != 0 &&
+	    strcmp(table, "security") != 0) {
 		pr_info_ratelimited("only valid in \'mangle\' or \'security\' table, not \'%s\'\n",
-				    par->table);
+				    table);
 		return -EINVAL;
 	}
 
@@ -116,25 +114,76 @@  static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
 	}
 }
 
-static struct xt_target secmark_tg_reg __read_mostly = {
-	.name       = "SECMARK",
-	.revision   = 0,
-	.family     = NFPROTO_UNSPEC,
-	.checkentry = secmark_tg_check,
-	.destroy    = secmark_tg_destroy,
-	.target     = secmark_tg,
-	.targetsize = sizeof(struct xt_secmark_target_info),
-	.me         = THIS_MODULE,
+static int secmark_tg_check_v1(const struct xt_tgchk_param *par)
+{
+	struct xt_secmark_target_info *info = par->targinfo;
+	struct xt_secmark_tginfo newinfo = {
+		.mode	= info->mode,
+	};
+	int ret;
+
+	memcpy(newinfo.secctx, info->secctx, SECMARK_SECCTX_MAX);
+
+	ret = secmark_tg_check(par->table, &newinfo);
+	info->secid = newinfo.secid;
+
+	return ret;
+}
+
+static unsigned int
+secmark_tg_v1(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct xt_secmark_target_info *info = par->targinfo;
+	struct xt_secmark_tginfo newinfo = {
+		.secid	= info->secid,
+	};
+
+	return secmark_tg(skb, &newinfo);
+}
+
+static int secmark_tg_check_v2(const struct xt_tgchk_param *par)
+{
+	return secmark_tg_check(par->table, par->targinfo);
+}
+
+static unsigned int
+secmark_tg_v2(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	return secmark_tg(skb, par->targinfo);
+}
+
+static struct xt_target secmark_tg_reg[] __read_mostly = {
+	{
+		.name		= "SECMARK",
+		.revision	= 0,
+		.family		= NFPROTO_UNSPEC,
+		.checkentry	= secmark_tg_check_v1,
+		.destroy	= secmark_tg_destroy,
+		.target		= secmark_tg_v1,
+		.targetsize	= sizeof(struct xt_secmark_target_info),
+		.me		= THIS_MODULE,
+	},
+	{
+		.name		= "SECMARK",
+		.revision	= 1,
+		.family		= NFPROTO_UNSPEC,
+		.checkentry	= secmark_tg_check_v2,
+		.destroy	= secmark_tg_destroy,
+		.target		= secmark_tg_v2,
+		.targetsize	= sizeof(struct xt_secmark_tginfo),
+		.usersize	= offsetof(struct xt_secmark_tginfo, secid),
+		.me		= THIS_MODULE,
+	},
 };
 
 static int __init secmark_tg_init(void)
 {
-	return xt_register_target(&secmark_tg_reg);
+	return xt_register_targets(secmark_tg_reg, ARRAY_SIZE(secmark_tg_reg));
 }
 
 static void __exit secmark_tg_exit(void)
 {
-	xt_unregister_target(&secmark_tg_reg);
+	xt_unregister_targets(secmark_tg_reg, ARRAY_SIZE(secmark_tg_reg));
 }
 
 module_init(secmark_tg_init);