netdev - Re: [PATCH net-next] net: warn if mac header was not set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <58f36d9554cb3dd19d52373f9496cd230ed48423.camel@redhat.com>
Date:   Tue, 21 Jun 2022 11:21:28 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev <netdev@...r.kernel.org>, Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next] net: warn if mac header was not set

Hello,

On Mon, 2022-06-20 at 02:30 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> 
> Make sure skb_mac_header(), skb_mac_offset() and skb_mac_header_len() uses
> are not fooled if the mac header has not been set.
> 
> These checks are enabled if CONFIG_DEBUG_NET=y
> 
> This commit will likely expose existing bugs in linux networking stacks.
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  include/linux/skbuff.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 82edf0359ab32d0d7cd583676b38d569ce0b24cc..cd4a8268894acce4bde16dc0fedb7eb13706f515 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2763,8 +2763,14 @@ static inline void skb_set_network_header(struct sk_buff *skb, const int offset)
>  	skb->network_header += offset;
>  }
>  
> +static inline int skb_mac_header_was_set(const struct sk_buff *skb)
> +{
> +	return skb->mac_header != (typeof(skb->mac_header))~0U;
> +}
> +
>  static inline unsigned char *skb_mac_header(const struct sk_buff *skb)
>  {
> +	DEBUG_NET_WARN_ON_ONCE(!skb_mac_header_was_set(skb));
>  	return skb->head + skb->mac_header;
>  }
>  
> @@ -2775,14 +2781,10 @@ static inline int skb_mac_offset(const struct sk_buff *skb)
>  
>  static inline u32 skb_mac_header_len(const struct sk_buff *skb)
>  {
> +	DEBUG_NET_WARN_ON_ONCE(!skb_mac_header_was_set(skb));

The patch LGTM. 

I'm wondering if it's worthy adding similar debug checks for network
and transport offset (with more patches). e.g. still in
skb_mac_header_len():

	DEBUG_NET_WARN_ON_ONCE(skb->network_header < skb->mac_header);

Thanks!

Paolo