netdev - Re: [PATCH net] drivers/net/wan/hdlc: In hdlc_rcv, check to make sure dev is an HDLC device

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: <20201019142226.4503ed65@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 19 Oct 2020 14:22:26 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Xie He <xie.he.0141@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Krzysztof Halasa <khc@...waw.pl>
Subject: Re: [PATCH net] drivers/net/wan/hdlc: In hdlc_rcv, check to make
 sure dev is an HDLC device

On Mon, 19 Oct 2020 03:49:42 -0700 Xie He wrote:
> The hdlc_rcv function is used as hdlc_packet_type.func to process any
> skb received in the kernel with skb->protocol == htons(ETH_P_HDLC).
> The purpose of this function is to provide second-stage processing for
> skbs not assigned a "real" L3 skb->protocol value in the first stage.
> 
> This function assumes the device from which the skb is received is an
> HDLC device (a device created by this module). It assumes that
> netdev_priv(dev) returns a pointer to "struct hdlc_device".
> 
> However, it is possible that some driver in the kernel (not necessarily
> in our control) submits a received skb with skb->protocol ==
> htons(ETH_P_HDLC), from a non-HDLC device. In this case, the skb would
> still be received by hdlc_rcv. This will cause problems.
> 
> hdlc_rcv should be able to recognize and drop invalid skbs. It should
> first make sure "dev" is actually an HDLC device, before starting its
> processing.
> 
> To reliably check if a device is an HDLC device, we can check if its
> dev->netdev_ops->ndo_start_xmit == hdlc_start_xmit, because all HDLC
> devices are required to set their ndo_start_xmit to hdlc_start_xmit
> (and all non-HDLC devices would not set their ndo_start_xmit to this).
> 
> Cc: Krzysztof Halasa <khc@...waw.pl>
> Signed-off-by: Xie He <xie.he.0141@...il.com>
> ---
>  drivers/net/wan/hdlc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wan/hdlc.c b/drivers/net/wan/hdlc.c
> index 9b00708676cf..0a392fb9aff8 100644
> --- a/drivers/net/wan/hdlc.c
> +++ b/drivers/net/wan/hdlc.c
> @@ -46,7 +46,15 @@ static struct hdlc_proto *first_proto;
>  static int hdlc_rcv(struct sk_buff *skb, struct net_device *dev,
>  		    struct packet_type *p, struct net_device *orig_dev)
>  {
> -	struct hdlc_device *hdlc = dev_to_hdlc(dev);
> +	struct hdlc_device *hdlc;
> +
> +	/* First make sure "dev" is an HDLC device */
> +	if (dev->netdev_ops->ndo_start_xmit != hdlc_start_xmit) {

Looks correct to me. I spotted there is also IFF_WAN_HDLC added by 
7cdc15f5f9db ("WAN: Generic HDLC now uses IFF_WAN_HDLC private flag.")
would using that flag also be correct and cleaner potentially? 

Up to you, just wanted to make sure you considered it.

> +		kfree_skb(skb);
> +		return NET_RX_SUCCESS;
> +	}
> +
> +	hdlc = dev_to_hdlc(dev);
>  
>  	if (!net_eq(dev_net(dev), &init_net)) {
>  		kfree_skb(skb);