Re: [PATCH 1/2 net-next v2] ipv4: validate IPV4_DEVCONF attributes properly

[Fernando Fernandez Mancera <fmancera@suse.de>]

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!