* [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout
@ 2021-04-29 13:39 Phil Sutter
2021-04-29 15:11 ` Jan Engelhardt
0 siblings, 1 reply; 4+ messages in thread
From: Phil Sutter @ 2021-04-29 13:39 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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(-)
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);
--
2.31.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout
2021-04-29 13:39 [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout Phil Sutter
@ 2021-04-29 15:11 ` Jan Engelhardt
2021-04-30 10:22 ` Phil Sutter
0 siblings, 1 reply; 4+ messages in thread
From: Jan Engelhardt @ 2021-04-29 15:11 UTC (permalink / raw)
To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout
2021-04-29 15:11 ` Jan Engelhardt
@ 2021-04-30 10:22 ` Phil Sutter
2021-04-30 10:23 ` Jan Engelhardt
0 siblings, 1 reply; 4+ messages in thread
From: Phil Sutter @ 2021-04-30 10:22 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Pablo Neira Ayuso, netfilter-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout
2021-04-30 10:22 ` Phil Sutter
@ 2021-04-30 10:23 ` Jan Engelhardt
0 siblings, 0 replies; 4+ messages in thread
From: Jan Engelhardt @ 2021-04-30 10:23 UTC (permalink / raw)
To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-30 10:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 13:39 [net-next PATCH] netfilter: xt_SECMARK: add new revision to fix structure layout Phil Sutter
2021-04-29 15:11 ` Jan Engelhardt
2021-04-30 10:22 ` Phil Sutter
2021-04-30 10:23 ` Jan Engelhardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.