Re: [PATCH nf-next] netfilter: nft_payload: extend offset to 65535 bytes

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

On 8/28/25 2:58 PM, Pablo Neira Ayuso wrote:
> On Thu, Aug 28, 2025 at 02:48:31PM +0200, Fernando Fernandez Mancera wrote:
>> In some situations 255 bytes offset is not enough to match or manipulate
>> the desired packet field. Increase the offset limit to 65535 or U16_MAX.
>>
>> In addition, the nla policy maximum value is not set anymore as it is
>> limited to s16. Instead, the maximum value is checked during the payload
>> expression initialization function.
>>
>> Tested with the nft command line tool.
>>
>> table ip filter {
>> 	chain output {
>> 		@nh,2040,8 set 0xff
>> 		@nh,524280,8 set 0xff
>> 		@nh,524280,8 0xff
>> 		@nh,2040,8 0xff
>> 	}
>> }
>>
>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>> ---
>>   include/net/netfilter/nf_tables_core.h |  2 +-
>>   net/netfilter/nft_payload.c            | 18 +++++++++++-------
>>   2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/netfilter/nf_tables_core.h b/include/net/netfilter/nf_tables_core.h
>> index 6c2f483d9828..7644cfe9267d 100644
>> --- a/include/net/netfilter/nf_tables_core.h
>> +++ b/include/net/netfilter/nf_tables_core.h
>> @@ -73,7 +73,7 @@ struct nft_ct {
>>   
>>   struct nft_payload {
>>   	enum nft_payload_bases	base:8;
>> -	u8			offset;
>> +	u16			offset;
>>   	u8			len;
>>   	u8			dreg;
>>   };
>> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
>> index 7dfc5343dae4..728a4c78775c 100644
>> --- a/net/netfilter/nft_payload.c
>> +++ b/net/netfilter/nft_payload.c
>> @@ -40,7 +40,7 @@ static bool nft_payload_rebuild_vlan_hdr(const struct sk_buff *skb, int mac_off,
>>   
>>   /* add vlan header into the user buffer for if tag was removed by offloads */
>>   static bool
>> -nft_payload_copy_vlan(u32 *d, const struct sk_buff *skb, u8 offset, u8 len)
>> +nft_payload_copy_vlan(u32 *d, const struct sk_buff *skb, u16 offset, u8 len)
>>   {
>>   	int mac_off = skb_mac_header(skb) - skb->data;
>>   	u8 *vlanh, *dst_u8 = (u8 *) d;
>> @@ -212,7 +212,7 @@ static const struct nla_policy nft_payload_policy[NFTA_PAYLOAD_MAX + 1] = {
>>   	[NFTA_PAYLOAD_SREG]		= { .type = NLA_U32 },
>>   	[NFTA_PAYLOAD_DREG]		= { .type = NLA_U32 },
>>   	[NFTA_PAYLOAD_BASE]		= { .type = NLA_U32 },
>> -	[NFTA_PAYLOAD_OFFSET]		= NLA_POLICY_MAX(NLA_BE32, 255),
>> +	[NFTA_PAYLOAD_OFFSET]		= { .type = NLA_BE32 },
> 
> Should this be
> 
>                                          NLA_POLICY_MAX(NLA_BE32, 65535),
> 
> ?
> 

Hi Pablo,

I don't think so. NLA_POLICY_MAX sets the nla_policy field "max" which 
is a 16 bit signed int (s16). Therefore, when doing 
NLA_POLICY_MAX(NLA_BE32, 65535) it triggers a warning as the max value 
set is actually "-1" in a s16.

This is why I decided to drop it. Let me know if I am missing something 
here..

> IIRC, NLA_POLICY_MAX(NLA_BE32, X) deprecates nft_parse_u32_check.
> 
>>   	[NFTA_PAYLOAD_LEN]		= NLA_POLICY_MAX(NLA_BE32, 255),
>>   	[NFTA_PAYLOAD_CSUM_TYPE]	= { .type = NLA_U32 },
>>   	[NFTA_PAYLOAD_CSUM_OFFSET]	= NLA_POLICY_MAX(NLA_BE32, 255),
>> @@ -797,7 +797,7 @@ static int nft_payload_csum_inet(struct sk_buff *skb, const u32 *src,
>>   
>>   struct nft_payload_set {
>>   	enum nft_payload_bases	base:8;
>> -	u8			offset;
>> +	u16			offset;
>>   	u8			len;
>>   	u8			sreg;
>>   	u8			csum_type;
>> @@ -812,7 +812,7 @@ struct nft_payload_vlan_hdr {
>>   };
>>   
>>   static bool
>> -nft_payload_set_vlan(const u32 *src, struct sk_buff *skb, u8 offset, u8 len,
>> +nft_payload_set_vlan(const u32 *src, struct sk_buff *skb, u16 offset, u8 len,
>>   		     int *vlan_hlen)
>>   {
>>   	struct nft_payload_vlan_hdr *vlanh;
>> @@ -940,14 +940,18 @@ static int nft_payload_set_init(const struct nft_ctx *ctx,
>>   				const struct nft_expr *expr,
>>   				const struct nlattr * const tb[])
>>   {
>> +	u32 csum_offset, offset, csum_type = NFT_PAYLOAD_CSUM_NONE;
>>   	struct nft_payload_set *priv = nft_expr_priv(expr);
>> -	u32 csum_offset, csum_type = NFT_PAYLOAD_CSUM_NONE;
>>   	int err;
>>   
>>   	priv->base        = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_BASE]));
>> -	priv->offset      = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_OFFSET]));
>>   	priv->len         = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_LEN]));
>>   
>> +	err = nft_parse_u32_check(tb[NFTA_PAYLOAD_OFFSET], U16_MAX, &offset);
>> +	if (err < 0)
>> +		return err;
>> +	priv->offset = offset;
> 
> Then, this nft_parse_u32_check() can go away.
> 
> IIRC, nft_parse_u32_check() was added before  NLA_POLICY_MAX(NLA_BE32, X)
> existed.
> 
> Can anyone please validate this claims?
> 
>> +
>>   	if (tb[NFTA_PAYLOAD_CSUM_TYPE])
>>   		csum_type = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_CSUM_TYPE]));
>>   	if (tb[NFTA_PAYLOAD_CSUM_OFFSET]) {
>> @@ -1069,7 +1073,7 @@ nft_payload_select_ops(const struct nft_ctx *ctx,
>>   	if (tb[NFTA_PAYLOAD_DREG] == NULL)
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	err = nft_parse_u32_check(tb[NFTA_PAYLOAD_OFFSET], U8_MAX, &offset);
>> +	err = nft_parse_u32_check(tb[NFTA_PAYLOAD_OFFSET], U16_MAX, &offset);
>>   	if (err < 0)
>>   		return ERR_PTR(err);
>>   
>> -- 
>> 2.51.0
>>