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

[ 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.

    2765 				break;
    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.

    2785 			}
    2786 			head = skb_peek(list);
    2787 			if (!head)
    2788 				break;
    2789 		}
    2790 		++seq;
    2791 	}
    2792 
    2793 	/* If we have a complete packet, copy it all into one skb. */
    2794 	if (tail != NULL) {
    2795 		/* If we have discarded any fragments,
    2796 		   signal a receive error. */
    2797 		if (PPP_MP_CB(head)->sequence != ppp->nextseq) {
    2798 			skb_queue_walk_safe(list, p, tmp) {
    2799 				if (p == head)
    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);
    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;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    2841 	}
    2842 
    2843 	return skb;
    2844 }

regards,
dan carpenter