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

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

[Xie He]

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@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.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) {
+		kfree_skb(skb);
+		return NET_RX_SUCCESS;
+	}
+
+	hdlc = dev_to_hdlc(dev);
 
 	if (!net_eq(dev_net(dev), &init_net)) {
 		kfree_skb(skb);
-- 
2.25.1

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

[Jakub Kicinski]

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@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.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);

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

[Xie He]

On Mon, Oct 19, 2020 at 2:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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.

Oh, Yes! I see IFF_WAN_HDLC is set for all HDLC devices. I also
searched through the kernel code and see no other uses of
IFF_WAN_HDLC. So I think we can use IFF_WAN_HDLC to reliably check if
a device is an HDLC device. This should be cleaner than checking
ndo_start_xmit.

Thanks! I'll change this patch to use IFF_WAN_HDLC.

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

[Jakub Kicinski]

On Mon, 19 Oct 2020 15:36:14 -0700 Xie He wrote:
> On Mon, Oct 19, 2020 at 2:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > 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.  
> 
> Oh, Yes! I see IFF_WAN_HDLC is set for all HDLC devices. I also
> searched through the kernel code and see no other uses of
> IFF_WAN_HDLC. So I think we can use IFF_WAN_HDLC to reliably check if
> a device is an HDLC device. This should be cleaner than checking
> ndo_start_xmit.
> 
> Thanks! I'll change this patch to use IFF_WAN_HDLC.

Cool! FWIW when you resend you can also trim the subject to just say:

net: hdlc: In hdlc_rcv, check to make sure dev is an HDLC device

There's no need for the full file path.

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

[Xie He]

On Mon, Oct 19, 2020 at 3:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Cool! FWIW when you resend you can also trim the subject to just say:
>
> net: hdlc: In hdlc_rcv, check to make sure dev is an HDLC device
>
> There's no need for the full file path.

OK. I'll do that. Thanks!