From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Date: Fri, 30 Jul 2021 08:48:33 +0000 Subject: Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors Message-Id: <20210730084833.GD25548@kadam> List-Id: References: <20210729141617.GC1267@kili> In-Reply-To: <20210729141617.GC1267@kili> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-ppp@vger.kernel.org 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