* [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly
@ 2026-02-26 13:39 Fernando Fernandez Mancera
2026-02-26 13:39 ` [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink Fernando Fernandez Mancera
2026-02-28 18:43 ` [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Jakub Kicinski
0 siblings, 2 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-02-26 13:39 UTC (permalink / raw)
To: netdev
Cc: tgraf, horms, pabeni, kuba, edumazet, dsahern, davem,
Fernando Fernandez Mancera
As the IPV4_DEVCONF netlink attributes are not being validated, it is
possible to use netlink to set read-only values like mc_forwarding. In
addition, valid ranges are not being validated neither but that is less
relevant as they aren't in sysctl.
To avoid similar situations in the future, define a NLA policy for
IPV4_DEVCONF attributes which are nested in IFLA_INET_CONF.
Please note that MEDIUM_ID is defined as NLA_U32 too because currently
its usage through netlink is broken for its valid value -1. Modifying
the type to NLA_S32 would break existing users of set/get netlink
operation.
Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: no changes
---
net/ipv4/devinet.c | 81 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 71 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 942a887bf089..590c68e979f5 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2063,12 +2063,76 @@ static const struct nla_policy inet_af_policy[IFLA_INET_MAX+1] = {
[IFLA_INET_CONF] = { .type = NLA_NESTED },
};
+static const struct nla_policy inet_devconf_policy[IPV4_DEVCONF_MAX + 1] = {
+ [IPV4_DEVCONF_FORWARDING] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_MC_FORWARDING] = { .type = NLA_REJECT },
+ [IPV4_DEVCONF_PROXY_ARP] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ACCEPT_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_SECURE_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_SEND_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_SHARED_MEDIA] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_RP_FILTER] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 2),
+ [IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_BOOTP_RELAY] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_LOG_MARTIANS] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_TAG] = { .type = NLA_U32 },
+ [IPV4_DEVCONF_ARPFILTER] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_U32 },
+ [IPV4_DEVCONF_NOXFRM] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_NOPOLICY] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_FORCE_IGMP_VERSION] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 3),
+ [IPV4_DEVCONF_ARP_ANNOUNCE] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 2),
+ [IPV4_DEVCONF_ARP_IGNORE] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 8),
+ [IPV4_DEVCONF_PROMOTE_SECONDARIES] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ARP_ACCEPT] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ARP_NOTIFY] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ACCEPT_LOCAL] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_SRC_VMARK] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_PROXY_ARP_PVLAN] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ROUTE_LOCALNET] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
+ [IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
+ [IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_DROP_GRATUITOUS_ARP] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_BC_FORWARDING] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+ [IPV4_DEVCONF_ARP_EVICT_NOCARRIER] = NLA_POLICY_RANGE(NLA_U32,
+ 0, 1),
+};
+
static int inet_validate_link_af(const struct net_device *dev,
const struct nlattr *nla,
struct netlink_ext_ack *extack)
{
- struct nlattr *a, *tb[IFLA_INET_MAX+1];
- int err, rem;
+ struct nlattr *tb[IFLA_INET_MAX + 1], *nested_tb[IPV4_DEVCONF_MAX + 1];
+ int err;
if (dev && !__in_dev_get_rtnl(dev))
return -EAFNOSUPPORT;
@@ -2079,15 +2143,12 @@ static int inet_validate_link_af(const struct net_device *dev,
return err;
if (tb[IFLA_INET_CONF]) {
- nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
- int cfgid = nla_type(a);
+ err = nla_parse_nested(nested_tb, IPV4_DEVCONF_MAX,
+ tb[IFLA_INET_CONF], inet_devconf_policy,
+ extack);
- if (nla_len(a) < 4)
- return -EINVAL;
-
- if (cfgid <= 0 || cfgid > IPV4_DEVCONF_MAX)
- return -EINVAL;
- }
+ if (err < 0)
+ return err;
}
return 0;
--
2.53.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink
2026-02-26 13:39 [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Fernando Fernandez Mancera
@ 2026-02-26 13:39 ` Fernando Fernandez Mancera
2026-02-28 18:45 ` Jakub Kicinski
2026-02-28 18:43 ` [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Jakub Kicinski
1 sibling, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-02-26 13:39 UTC (permalink / raw)
To: netdev
Cc: tgraf, horms, pabeni, kuba, edumazet, dsahern, davem,
Fernando Fernandez Mancera
When modifying IPv4 devconf values using netlink for some relevant
fields the rt_cache_flush() call was missing. In addition, if forwarding
is enabled on the interface then disable LRO.
This is needed to avoid possible connectivity issues and ease the
responsabilities of user space tools.
Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: use netif_disable_lro() as we already hold netdev_need_ops_lock()
and use net_device struct passed as argument instead of using in_dev->dev
---
net/ipv4/devinet.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 590c68e979f5..b1651da3f572 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2159,6 +2159,8 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
{
struct in_device *in_dev = __in_dev_get_rtnl(dev);
struct nlattr *a, *tb[IFLA_INET_MAX+1];
+ struct net *net = dev_net(dev);
+ bool flush_cache = false;
int rem;
if (!in_dev)
@@ -2168,8 +2170,27 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
return -EINVAL;
if (tb[IFLA_INET_CONF]) {
- nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
+ nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+
+ switch (nla_type(a)) {
+ case IPV4_DEVCONF_FORWARDING:
+ if (nla_get_u32(a))
+ netif_disable_lro(dev);
+ fallthrough;
+ case IPV4_DEVCONF_NOXFRM:
+ case IPV4_DEVCONF_NOPOLICY:
+ case IPV4_DEVCONF_PROMOTE_SECONDARIES:
+ case IPV4_DEVCONF_ROUTE_LOCALNET:
+ case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
+ flush_cache = true;
+ break;
+ default:
+ break;
+ }
+ }
+ if (flush_cache)
+ rt_cache_flush(net);
}
return 0;
--
2.53.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink
2026-02-26 13:39 ` [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink Fernando Fernandez Mancera
@ 2026-02-28 18:45 ` Jakub Kicinski
2026-03-02 8:27 ` Fernando Fernandez Mancera
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-02-28 18:45 UTC (permalink / raw)
To: Fernando Fernandez Mancera
Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On Thu, 26 Feb 2026 14:39:49 +0100 Fernando Fernandez Mancera wrote:
> When modifying IPv4 devconf values using netlink for some relevant
> fields the rt_cache_flush() call was missing. In addition, if forwarding
> is enabled on the interface then disable LRO.
>
> This is needed to avoid possible connectivity issues and ease the
> responsabilities of user space tools.
>
> Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
This OTOH probably deserves to be a real fix for net?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink
2026-02-28 18:45 ` Jakub Kicinski
@ 2026-03-02 8:27 ` Fernando Fernandez Mancera
2026-03-03 0:19 ` Jakub Kicinski
0 siblings, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-03-02 8:27 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On 2/28/26 7:45 PM, Jakub Kicinski wrote:
> On Thu, 26 Feb 2026 14:39:49 +0100 Fernando Fernandez Mancera wrote:
>> When modifying IPv4 devconf values using netlink for some relevant
>> fields the rt_cache_flush() call was missing. In addition, if forwarding
>> is enabled on the interface then disable LRO.
>>
>> This is needed to avoid possible connectivity issues and ease the
>> responsabilities of user space tools.
>>
>> Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>
> This OTOH probably deserves to be a real fix for net?
Okay, makes sense. Let me re-send this to net.git without relying on the
previous patch. Then once is merged and net-next is synced I will
re-send the validation one.
Thanks,
Fernando.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink
2026-03-02 8:27 ` Fernando Fernandez Mancera
@ 2026-03-03 0:19 ` Jakub Kicinski
2026-03-03 11:06 ` Fernando Fernandez Mancera
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-03-03 0:19 UTC (permalink / raw)
To: Fernando Fernandez Mancera
Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On Mon, 2 Mar 2026 09:27:04 +0100 Fernando Fernandez Mancera wrote:
> On 2/28/26 7:45 PM, Jakub Kicinski wrote:
> > On Thu, 26 Feb 2026 14:39:49 +0100 Fernando Fernandez Mancera wrote:
> >> When modifying IPv4 devconf values using netlink for some relevant
> >> fields the rt_cache_flush() call was missing. In addition, if forwarding
> >> is enabled on the interface then disable LRO.
> >>
> >> This is needed to avoid possible connectivity issues and ease the
> >> responsabilities of user space tools.
> >>
> >> Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
> >> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> >
> > This OTOH probably deserves to be a real fix for net?
>
> Okay, makes sense. Let me re-send this to net.git without relying on the
> previous patch. Then once is merged and net-next is synced I will
> re-send the validation one.
I could be wrong, but FWIW in this case I think the changes won't
conflict, so you could also post them at once against different trees.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink
2026-03-03 0:19 ` Jakub Kicinski
@ 2026-03-03 11:06 ` Fernando Fernandez Mancera
0 siblings, 0 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-03-03 11:06 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On 3/3/26 1:19 AM, Jakub Kicinski wrote:
> On Mon, 2 Mar 2026 09:27:04 +0100 Fernando Fernandez Mancera wrote:
>> On 2/28/26 7:45 PM, Jakub Kicinski wrote:
>>> On Thu, 26 Feb 2026 14:39:49 +0100 Fernando Fernandez Mancera wrote:
>>>> When modifying IPv4 devconf values using netlink for some relevant
>>>> fields the rt_cache_flush() call was missing. In addition, if forwarding
>>>> is enabled on the interface then disable LRO.
>>>>
>>>> This is needed to avoid possible connectivity issues and ease the
>>>> responsabilities of user space tools.
>>>>
>>>> Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
>>>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>>>
>>> This OTOH probably deserves to be a real fix for net?
>>
>> Okay, makes sense. Let me re-send this to net.git without relying on the
>> previous patch. Then once is merged and net-next is synced I will
>> re-send the validation one.
>
> I could be wrong, but FWIW in this case I think the changes won't
> conflict, so you could also post them at once against different trees.
>
I already posted the change against net tree. Will post the one for
net-next later after some testing.
Thank you Jakub for all the feedback,
Fernando.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly
2026-02-26 13:39 [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Fernando Fernandez Mancera
2026-02-26 13:39 ` [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink Fernando Fernandez Mancera
@ 2026-02-28 18:43 ` Jakub Kicinski
2026-03-02 8:35 ` Fernando Fernandez Mancera
1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-02-28 18:43 UTC (permalink / raw)
To: Fernando Fernandez Mancera
Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On Thu, 26 Feb 2026 14:39:48 +0100 Fernando Fernandez Mancera wrote:
> As the IPV4_DEVCONF netlink attributes are not being validated, it is
> possible to use netlink to set read-only values like mc_forwarding. In
> addition, valid ranges are not being validated neither but that is less
> relevant as they aren't in sysctl.
>
> To avoid similar situations in the future, define a NLA policy for
> IPV4_DEVCONF attributes which are nested in IFLA_INET_CONF.
Very nice, I think we should drop the Fixes tag tho.
Adding missed validation is always tricky, we don't really want people
to backport this to stable releases, the risk of regression (of broken
user space) is too high. Unless there's some crash this prevents, in
which case we'd need a more targeted fix for just those values in net.
> Please note that MEDIUM_ID is defined as NLA_U32 too because currently
> its usage through netlink is broken for its valid value -1. Modifying
> the type to NLA_S32 would break existing users of set/get netlink
> operation.
Say more? The policy type not matching the accessor used by the kernel
is probably fine in this case (since there's a common accessor used for
all attrs). If it helps the policy, we can use a different type.
> +static const struct nla_policy inet_devconf_policy[IPV4_DEVCONF_MAX + 1] = {
> + [IPV4_DEVCONF_FORWARDING] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_MC_FORWARDING] = { .type = NLA_REJECT },
> + [IPV4_DEVCONF_PROXY_ARP] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_ACCEPT_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_SECURE_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_SEND_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_SHARED_MEDIA] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_RP_FILTER] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 2),
> + [IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_BOOTP_RELAY] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_LOG_MARTIANS] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_TAG] = { .type = NLA_U32 },
> + [IPV4_DEVCONF_ARPFILTER] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_U32 },
> + [IPV4_DEVCONF_NOXFRM] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_NOPOLICY] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_FORCE_IGMP_VERSION] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 3),
> + [IPV4_DEVCONF_ARP_ANNOUNCE] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 2),
> + [IPV4_DEVCONF_ARP_IGNORE] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 8),
> + [IPV4_DEVCONF_PROMOTE_SECONDARIES] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_ARP_ACCEPT] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_ARP_NOTIFY] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_ACCEPT_LOCAL] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_SRC_VMARK] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_PROXY_ARP_PVLAN] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_ROUTE_LOCALNET] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
> + [IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
> + [IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
> + [IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN] = NLA_POLICY_RANGE(NLA_U32,
> + 0, 1),
The indentation is rather awkward, please adjust to fit the common case
on one line and special case the long ones.
// mis-adjust when needed
[IPV4_DEVCONF_PROMOTE_SECONDARIES] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
// common / normal case
[IPV4_DEVCONF_ARP_ACCEPT] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
[IPV4_DEVCONF_ARP_NOTIFY] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
[IPV4_DEVCONF_ACCEPT_LOCAL] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
...
// overflow type fully to next line if doesn't fit even mis-adjusted
[IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL] =
{ .type = NLA_U32 },
[IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL] =
{ .type = NLA_U32 },
[IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN] =
NLA_POLICY_RANGE(NLA_U32, 0, 1),
--
pw-bot: cr
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly
2026-02-28 18:43 ` [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Jakub Kicinski
@ 2026-03-02 8:35 ` Fernando Fernandez Mancera
2026-03-03 0:18 ` Jakub Kicinski
0 siblings, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-03-02 8:35 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On 2/28/26 7:43 PM, Jakub Kicinski wrote:
> On Thu, 26 Feb 2026 14:39:48 +0100 Fernando Fernandez Mancera wrote:
>> As the IPV4_DEVCONF netlink attributes are not being validated, it is
>> possible to use netlink to set read-only values like mc_forwarding. In
>> addition, valid ranges are not being validated neither but that is less
>> relevant as they aren't in sysctl.
>>
>> To avoid similar situations in the future, define a NLA policy for
>> IPV4_DEVCONF attributes which are nested in IFLA_INET_CONF.
>
> Very nice, I think we should drop the Fixes tag tho.
> Adding missed validation is always tricky, we don't really want people
> to backport this to stable releases, the risk of regression (of broken
> user space) is too high. Unless there's some crash this prevents, in
> which case we'd need a more targeted fix for just those values in net.
>
>> Please note that MEDIUM_ID is defined as NLA_U32 too because currently
>> its usage through netlink is broken for its valid value -1. Modifying
>> the type to NLA_S32 would break existing users of set/get netlink
>> operation.
>
> Say more? The policy type not matching the accessor used by the kernel
> is probably fine in this case (since there's a common accessor used for
> all attrs). If it helps the policy, we can use a different type.
>
The problem is not only not matching the accessor.. the problem is that
while it was not validated if users were using NLA_U32 as indicated by
the original implementation (see blamed commit), this would break them.
Is it one option to set the type to NLA_S32 and wait to see if someone
complains? I am not sure how many people might be using it considering
the type is wrong.
Thanks,
Fernando.
>> +static const struct nla_policy inet_devconf_policy[IPV4_DEVCONF_MAX + 1] = {
>> + [IPV4_DEVCONF_FORWARDING] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_MC_FORWARDING] = { .type = NLA_REJECT },
>> + [IPV4_DEVCONF_PROXY_ARP] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_ACCEPT_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_SECURE_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_SEND_REDIRECTS] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_SHARED_MEDIA] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_RP_FILTER] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 2),
>> + [IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_BOOTP_RELAY] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_LOG_MARTIANS] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_TAG] = { .type = NLA_U32 },
>> + [IPV4_DEVCONF_ARPFILTER] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_U32 },
>> + [IPV4_DEVCONF_NOXFRM] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_NOPOLICY] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_FORCE_IGMP_VERSION] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 3),
>> + [IPV4_DEVCONF_ARP_ANNOUNCE] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 2),
>> + [IPV4_DEVCONF_ARP_IGNORE] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 8),
>> + [IPV4_DEVCONF_PROMOTE_SECONDARIES] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_ARP_ACCEPT] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_ARP_NOTIFY] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_ACCEPT_LOCAL] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_SRC_VMARK] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_PROXY_ARP_PVLAN] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_ROUTE_LOCALNET] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>> + [IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
>> + [IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL] = { .type = NLA_U32 },
>> + [IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN] = NLA_POLICY_RANGE(NLA_U32,
>> + 0, 1),
>
> The indentation is rather awkward, please adjust to fit the common case
> on one line and special case the long ones.
>
> // mis-adjust when needed
> [IPV4_DEVCONF_PROMOTE_SECONDARIES] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
> // common / normal case
> [IPV4_DEVCONF_ARP_ACCEPT] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
> [IPV4_DEVCONF_ARP_NOTIFY] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
> [IPV4_DEVCONF_ACCEPT_LOCAL] = NLA_POLICY_RANGE(NLA_U32, 0, 1),
> ...
> // overflow type fully to next line if doesn't fit even mis-adjusted
> [IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL] =
> { .type = NLA_U32 },
> [IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL] =
> { .type = NLA_U32 },
> [IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN] =
> NLA_POLICY_RANGE(NLA_U32, 0, 1),
Thanks for the suggestion!
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly
2026-03-02 8:35 ` Fernando Fernandez Mancera
@ 2026-03-03 0:18 ` Jakub Kicinski
2026-03-03 11:04 ` Fernando Fernandez Mancera
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-03-03 0:18 UTC (permalink / raw)
To: Fernando Fernandez Mancera
Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On Mon, 2 Mar 2026 09:35:06 +0100 Fernando Fernandez Mancera wrote:
> >> Please note that MEDIUM_ID is defined as NLA_U32 too because currently
> >> its usage through netlink is broken for its valid value -1. Modifying
> >> the type to NLA_S32 would break existing users of set/get netlink
> >> operation.
> >
> > Say more? The policy type not matching the accessor used by the kernel
> > is probably fine in this case (since there's a common accessor used for
> > all attrs). If it helps the policy, we can use a different type.
>
> The problem is not only not matching the accessor.. the problem is that
> while it was not validated if users were using NLA_U32 as indicated by
> the original implementation (see blamed commit), this would break them.
>
> Is it one option to set the type to NLA_S32 and wait to see if someone
> complains? I am not sure how many people might be using it considering
> the type is wrong.
Sorry, I'm not connecting the dots.
[IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_U32 },
vs
[IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_S32 },
makes absolutely _no_ difference. For bare types with no ranges
the validation will only check the attr is 4B which is identical
for both.
For S32 you can add a range, however, the NLA_POLICY_RANGE() takes
two signed 16b values. So a range of -1 .. x is completely fine.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly
2026-03-03 0:18 ` Jakub Kicinski
@ 2026-03-03 11:04 ` Fernando Fernandez Mancera
0 siblings, 0 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2026-03-03 11:04 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, tgraf, horms, pabeni, edumazet, dsahern, davem
On 3/3/26 1:18 AM, Jakub Kicinski wrote:
> On Mon, 2 Mar 2026 09:35:06 +0100 Fernando Fernandez Mancera wrote:
>>>> Please note that MEDIUM_ID is defined as NLA_U32 too because currently
>>>> its usage through netlink is broken for its valid value -1. Modifying
>>>> the type to NLA_S32 would break existing users of set/get netlink
>>>> operation.
>>>
>>> Say more? The policy type not matching the accessor used by the kernel
>>> is probably fine in this case (since there's a common accessor used for
>>> all attrs). If it helps the policy, we can use a different type.
>>
>> The problem is not only not matching the accessor.. the problem is that
>> while it was not validated if users were using NLA_U32 as indicated by
>> the original implementation (see blamed commit), this would break them.
>>
>> Is it one option to set the type to NLA_S32 and wait to see if someone
>> complains? I am not sure how many people might be using it considering
>> the type is wrong.
>
> Sorry, I'm not connecting the dots.
>
> [IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_U32 },
> vs
> [IPV4_DEVCONF_MEDIUM_ID] = { .type = NLA_S32 },
>
> makes absolutely _no_ difference. For bare types with no ranges
> the validation will only check the attr is 4B which is identical
> for both.
>
I was not connecting the dots neither. Thanks for explaining, I already
knew this but somehow I forgot about it when working on this.
> For S32 you can add a range, however, the NLA_POLICY_RANGE() takes
> two signed 16b values. So a range of -1 .. x is completely fine.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-03-03 11:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-26 13:39 [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Fernando Fernandez Mancera
2026-02-26 13:39 ` [PATCH 2/2 net-next v2] ipv4: bump rt_genid when a relevant devconf value changes through netlink Fernando Fernandez Mancera
2026-02-28 18:45 ` Jakub Kicinski
2026-03-02 8:27 ` Fernando Fernandez Mancera
2026-03-03 0:19 ` Jakub Kicinski
2026-03-03 11:06 ` Fernando Fernandez Mancera
2026-02-28 18:43 ` [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly Jakub Kicinski
2026-03-02 8:35 ` Fernando Fernandez Mancera
2026-03-03 0:18 ` Jakub Kicinski
2026-03-03 11:04 ` Fernando Fernandez Mancera
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox