Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors

[Dan Carpenter <dan.carpenter@oracle.com>]

Hi James,

Thanks for your response.  This is a new not yet published Smatch check.
I reported the bug wrong, it's complaining about the other kfree_skb().
See below.

Smatch understands about break statements.  :P

On Thu, Jul 29, 2021 at 05:16:17PM +0300, Dan Carpenter wrote:
> [ This is ancient code, but the warning seems somewhat reasonable and
>   hopefully not too complicated to review? - dan ]
> 
> Hello PPP devs,
> 
> The patch 8a49ad6e89fe: "ppp: fix 'ppp_mp_reconstruct bad seq'
> errors" from Feb 24, 2012, leads to the following static checker
> warning:
> 
> 	drivers/net/ppp/ppp_generic.c:2840 ppp_mp_reconstruct()
> 	error: dereferencing freed memory 'tail'
> 
> drivers/net/ppp/ppp_generic.c
>     2692 static struct sk_buff *
>     2693 ppp_mp_reconstruct(struct ppp *ppp)
>     2694 {
>     2695 	u32 seq = ppp->nextseq;
>     2696 	u32 minseq = ppp->minseq;
>     2697 	struct sk_buff_head *list = &ppp->mrq;
>     2698 	struct sk_buff *p, *tmp;
>     2699 	struct sk_buff *head, *tail;
>     2700 	struct sk_buff *skb = NULL;
>     2701 	int lost = 0, len = 0;
>     2702 
>     2703 	if (ppp->mrru = 0)	/* do nothing until mrru is set */
>     2704 		return NULL;
>     2705 	head = __skb_peek(list);
>     2706 	tail = NULL;
>     2707 	skb_queue_walk_safe(list, p, tmp) {
>     2708 	again:
>     2709 		if (seq_before(PPP_MP_CB(p)->sequence, seq)) {
>     2710 			/* this can't happen, anyway ignore the skb */
>     2711 			netdev_err(ppp->dev, "ppp_mp_reconstruct bad "
>     2712 				   "seq %u < %u\n",
>     2713 				   PPP_MP_CB(p)->sequence, seq);
>     2714 			__skb_unlink(p, list);
>     2715 			kfree_skb(p);
>     2716 			continue;
>     2717 		}
>     2718 		if (PPP_MP_CB(p)->sequence != seq) {
>     2719 			u32 oldseq;
>     2720 			/* Fragment `seq' is missing.  If it is after
>     2721 			   minseq, it might arrive later, so stop here. */
>     2722 			if (seq_after(seq, minseq))
>     2723 				break;
>     2724 			/* Fragment `seq' is lost, keep going. */
>     2725 			lost = 1;
>     2726 			oldseq = seq;
>     2727 			seq = seq_before(minseq, PPP_MP_CB(p)->sequence)?
>     2728 				minseq + 1: PPP_MP_CB(p)->sequence;
>     2729 
>     2730 			if (ppp->debug & 1)
>     2731 				netdev_printk(KERN_DEBUG, ppp->dev,
>     2732 					      "lost frag %u..%u\n",
>     2733 					      oldseq, seq-1);
>     2734 
>     2735 			goto again;
>     2736 		}
>     2737 
>     2738 		/*
>     2739 		 * At this point we know that all the fragments from
>     2740 		 * ppp->nextseq to seq are either present or lost.
>     2741 		 * Also, there are no complete packets in the queue
>     2742 		 * that have no missing fragments and end before this
>     2743 		 * fragment.
>     2744 		 */
>     2745 
>     2746 		/* B bit set indicates this fragment starts a packet */
>     2747 		if (PPP_MP_CB(p)->BEbits & B) {
>     2748 			head = p;
>     2749 			lost = 0;
>     2750 			len = 0;
>     2751 		}
>     2752 
>     2753 		len += p->len;
>     2754 
>     2755 		/* Got a complete packet yet? */
>     2756 		if (lost = 0 && (PPP_MP_CB(p)->BEbits & E) &&
>     2757 		    (PPP_MP_CB(head)->BEbits & B)) {
>     2758 			if (len > ppp->mrru + 2) {
>     2759 				++ppp->dev->stats.rx_length_errors;
>     2760 				netdev_printk(KERN_DEBUG, ppp->dev,
>     2761 					      "PPP: reconstructed packet"
>     2762 					      " is too long (%d)\n", len);
>     2763 			} else {
>     2764 				tail = p;
>                                         ^^^^^^^^
> tail is set to p.

At this point Smatch understands that "tail" and "p" are non-NULL.

> 
>     2765 				break;

We hit the break statement.

>     2766 			}
>     2767 			ppp->nextseq = seq + 1;
>     2768 		}
>     2769 
>     2770 		/*
>     2771 		 * If this is the ending fragment of a packet,
>     2772 		 * and we haven't found a complete valid packet yet,
>     2773 		 * we can discard up to and including this fragment.
>     2774 		 */
>     2775 		if (PPP_MP_CB(p)->BEbits & E) {
>                             ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> If "tail" is set, can this condition be true?
> 
>     2776 			struct sk_buff *tmp2;
>     2777 
>     2778 			skb_queue_reverse_walk_from_safe(list, p, tmp2) {
>     2779 				if (ppp->debug & 1)
>     2780 					netdev_printk(KERN_DEBUG, ppp->dev,
>     2781 						      "discarding frag %u\n",
>     2782 						      PPP_MP_CB(p)->sequence);
>     2783 				__skb_unlink(p, list);
>     2784 				kfree_skb(p);
>                                         ^^^^^^^^^^^^
> On the first iteration through the loop then "p" is still set to "tail"
> so this will free "tail", leading to problems down the line.

I was just completely wrong to write this.

> 
>     2785 			}
>     2786 			head = skb_peek(list);
>     2787 			if (!head)
>     2788 				break;
>     2789 		}
>     2790 		++seq;
>     2791 	}

We jump to here.

>     2792 
>     2793 	/* If we have a complete packet, copy it all into one skb. */
>     2794 	if (tail != NULL) {

This condition means "tail = p"

>     2795 		/* If we have discarded any fragments,
>     2796 		   signal a receive error. */
>     2797 		if (PPP_MP_CB(head)->sequence != ppp->nextseq) {

Smatch is supposed to "understand" condtions, but this one is quite
complicated and the only thing that Smatch understands is just the
basic meaning that these two are not equal.

>     2798 			skb_queue_walk_safe(list, p, tmp) {
>     2799 				if (p = head)

One of the weak points of Smatch is how it parses lists...  Also it
doesn't have any implications for this if (p = head) condition.

>     2800 					break;
>     2801 				if (ppp->debug & 1)
>     2802 					netdev_printk(KERN_DEBUG, ppp->dev,
>     2803 						      "discarding frag %u\n",
>     2804 						      PPP_MP_CB(p)->sequence);
>     2805 				__skb_unlink(p, list);
>     2806 				kfree_skb(p);

We know that p = tail going in to the start of this list so this is
going to free tail.  Of course kfree_skb() is refcounted and the free
only happens when the last reference is dropped.

>     2807 			}
>     2808 
>     2809 			if (ppp->debug & 1)
>     2810 				netdev_printk(KERN_DEBUG, ppp->dev,
>     2811 					      "  missed pkts %u..%u\n",
>     2812 					      ppp->nextseq,
>     2813 					      PPP_MP_CB(head)->sequence-1);
>     2814 			++ppp->dev->stats.rx_dropped;
>     2815 			ppp_receive_error(ppp);
>     2816 		}
>     2817 
>     2818 		skb = head;
>     2819 		if (head != tail) {
>     2820 			struct sk_buff **fragpp = &skb_shinfo(skb)->frag_list;
>     2821 			p = skb_queue_next(list, head);
>     2822 			__skb_unlink(skb, list);
>     2823 			skb_queue_walk_from_safe(list, p, tmp) {
>     2824 				__skb_unlink(p, list);
>     2825 				*fragpp = p;
>     2826 				p->next = NULL;
>     2827 				fragpp = &p->next;
>     2828 
>     2829 				skb->len += p->len;
>     2830 				skb->data_len += p->len;
>     2831 				skb->truesize += p->truesize;
>     2832 
>     2833 				if (p = tail)
>     2834 					break;
>     2835 			}
>     2836 		} else {
>     2837 			__skb_unlink(skb, list);
>     2838 		}
>     2839 
> --> 2840 		ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here is where Smatch complains.

> 
>     2841 	}
>     2842 
>     2843 	return skb;
>     2844 }
> 
> regards,
> dan carpenter