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