From: Dan Carpenter <dan.carpenter@oracle.com>
To: linux-ppp@vger.kernel.org
Subject: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
Date: Thu, 29 Jul 2021 14:16:17 +0000 [thread overview]
Message-ID: <20210729141617.GC1267@kili> (raw)
[ 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
next reply other threads:[~2021-07-29 14:16 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 14:16 Dan Carpenter [this message]
2021-07-29 21:08 ` [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors James Carlson
2021-07-30 8:48 ` Dan Carpenter
2021-07-30 17:15 ` James Carlson
2021-07-31 18:36 ` James Carlson
2021-08-02 11:43 ` Dan Carpenter
2021-08-02 12:37 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210729141617.GC1267@kili \
--to=dan.carpenter@oracle.com \
--cc=linux-ppp@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.