netdev - Re: [PATCH] drivers/net/wan/x25

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: <4b46b0f6-8424-5a6e-a4ae-3729f54c5d4b@gmail.com>
Date:   Mon, 13 Jul 2020 14:21:18 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Xie He <xie.he.0141@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Shannon Nelson <snelson@...sando.io>,
        Martin Habets <mhabets@...arflare.com>,
        "Michael S. Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-x25@...r.kernel.org
Subject: Re: [PATCH] drivers/net/wan/x25_asy: Fix to make it work



On 7/7/20 9:37 PM, Xie He wrote:
> This driver is not working because of problems of its receiving code.
> This patch fixes it to make it work.
> 
> When the driver receives an LAPB frame, it should first pass the frame
> to the LAPB module to process. After processing, the LAPB module passes
> the data (the packet) back to the driver, the driver should then add a
> one-byte pseudo header and pass the data to upper layers.
> 
> The changes to the "x25_asy_bump" function and the
> "x25_asy_data_indication" function are to correctly implement this
> procedure.
> 
> Also, the "x25_asy_unesc" function ignores any frame that is shorter
> than 3 bytes. However the shortest frames are 2-byte long. So we need
> to change it to allow 2-byte frames to pass.
> 
> Signed-off-by: Xie He <xie.he.0141@...il.com>
> ---
>  drivers/net/wan/x25_asy.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
> index 69773d228ec1..3fd8938e591b 100644
> --- a/drivers/net/wan/x25_asy.c
> +++ b/drivers/net/wan/x25_asy.c
> @@ -183,7 +183,7 @@ static inline void x25_asy_unlock(struct x25_asy *sl)
>  	netif_wake_queue(sl->dev);
>  }
>  
> -/* Send one completely decapsulated IP datagram to the IP layer. */
> +/* Send an LAPB frame to the LAPB module to process. */
>  
>  static void x25_asy_bump(struct x25_asy *sl)
>  {
> @@ -195,13 +195,12 @@ static void x25_asy_bump(struct x25_asy *sl)
>  	count = sl->rcount;
>  	dev->stats.rx_bytes += count;
>  
> -	skb = dev_alloc_skb(count+1);
> +	skb = dev_alloc_skb(count);
>  	if (skb == NULL) {
>  		netdev_warn(sl->dev, "memory squeeze, dropping packet\n");
>  		dev->stats.rx_dropped++;
>  		return;
>  	}
> -	skb_push(skb, 1);	/* LAPB internal control */
>  	skb_put_data(skb, sl->rbuff, count);
>  	skb->protocol = x25_type_trans(skb, sl->dev);
>  	err = lapb_data_received(skb->dev, skb);
> @@ -209,7 +208,6 @@ static void x25_asy_bump(struct x25_asy *sl)
>  		kfree_skb(skb);
>  		printk(KERN_DEBUG "x25_asy: data received err - %d\n", err);
>  	} else {
> -		netif_rx(skb);
>  		dev->stats.rx_packets++;
>  	}
>  }
> @@ -356,12 +354,16 @@ static netdev_tx_t x25_asy_xmit(struct sk_buff *skb,
>   */
>  
>  /*
> - *	Called when I frame data arrives. We did the work above - throw it
> - *	at the net layer.
> + *	Called when I frame data arrives. We add a pseudo header for upper
> + *	layers and pass it to upper layers.
>   */
>  
>  static int x25_asy_data_indication(struct net_device *dev, struct sk_buff *skb)
>  {

It is not clear to me what guarantee we have to have one byte of headroom in the skb
at this point.

You might add to be safe : (as done in lapbeth_data_indication(), but after the skb_push() which seems wrong)

      if (skb_cow(skb, 1)) {
            kfree_skb(skb); /* This line I am not sure, but looking at
                             * lapb_data_indication() this might be needed.
                             */
	    return NET_RX_DROP;
      }

> +	skb_push(skb, 1);
> +	skb->data[0] = X25_IFACE_DATA;
> +	skb->protocol = x25_type_trans(skb, dev);
> +
>  	return netif_rx(skb);
>  }
>  
> @@ -657,7 +659,7 @@ static void x25_asy_unesc(struct x25_asy *sl, unsigned char s)
>  	switch (s) {
>  	case X25_END:
>  		if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
> -		    sl->rcount > 2)
> +		    sl->rcount >= 2)
>  			x25_asy_bump(sl);
>  		clear_bit(SLF_ESCAPE, &sl->flags);
>  		sl->rcount = 0;
>