ppp: fix 'ppp_mp_reconstruct bad seq' errors

bug report Dan Carpenter 2 participants 7 messages Jul 29, 2021 – Aug 2, 2021 PPP protocol drivers and compressors

Share & tools

James Carlson

2021-07-29 21:08 UTC #1 lore raw

Quoted 6 lines · Dan Carpenter

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'

What's the static checker, and does it provide any deeper analysis of the code path and branch assumptions involved?

Quoted 26 lines

    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) {
                            ^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

If "tail" is set, can this condition be true?

No. You can't get here at all if tail is set. Note the break at line 2765, which takes us out of the "skb_queue_walk_safe" iteration. That takes us all the way down to line 2793.

Quoted 10 lines

    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);
                                        ^^^^^^^^^^^^

Quoted 2 lines

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.

tail must be NULL here. Otherwise, we would have broken out of the loop at line 2765.

Other than that the code is a bit hard to read, I don't see a problem here.

Signature · 1 line

James Carlson         42.703N 71.076W         <[email protected]>

James Carlson

2021-07-30 17:15 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 13 lines · Dan Carpenter

Quoted 11 lines

    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;
                                        ^^^^^^^^

Quoted 1 line

tail is set to p.

Quoted 1 line

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

Yep. And 'head' is non-NULL and points to the first buf of the reassembled packet, 'tail' is non-NULL and points to the last buf of the reassembled packet. And head may be equal to tail if it's packet consisting of a single MP fragment. And because 'lost' is zero, we know that we have all of the intermediate fragments chained as well. It's a complete message.

Quoted 4 lines

Quoted 2 lines
2793 /* If we have a complete packet, copy it all into one skb. */
2794 if (tail != NULL) {
This condition means "tail = p"

True at this point. (Not real meaningful, as we'll see in a bit, but true nonetheless.)

Quoted 7 lines

Quoted 3 lines
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.

That's ok; it's a worthwhile branch to explore, so we can assume it's true.

Quoted 5 lines

Quoted 2 lines
    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.

This is where things break down. That queue walker macro on line 2798 re-assigns 'p'. The code marches over the list and says "anything that still exists up to (but not including) the head for this completed packet is trash." Note that *NOTHING* here is harming 'head' or anything in the list that follows that buffer -- which includes 'tail.'

Quoted 1 line

Quoted 1 line
    2800 					break;

That break protects us from hurting 'tail'.

Quoted 10 lines

Quoted 6 lines
    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.

Not so. p != tail here. It cannot possibly be tail, because we (A) reassigned 'p' at the top of the loop and (B) broke out of the loop on hitting 'head'.

Quoted 8 lines

Quoted 6 lines

    2836 		} else {
    2837 			__skb_unlink(skb, list);
    2838 		}
    2839 
--> 2840 		ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here is where Smatch complains.

If that's Smatch's analysis of the situation, then Smatch is wrong. It's a bogus warning.

Signature · 1 line

James Carlson         42.703N 71.076W         <[email protected]>

James Carlson

2021-07-31 18:36 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 8 lines · James Carlson

Quoted 4 lines · Dan Carpenter
Quoted 2 lines
--> 2840 		ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is where Smatch complains.
If that's Smatch's analysis of the situation, then Smatch is wrong. It's a bogus warning.

For what it's worth, it's not my code, and I agree that it's at least a bit hard to follow, and may well harbor bugs. If you're suggesting either some kind of suppression directive (I tried looking for some Smatch documentation but couldn't find much to suggest that's possible, though I guess now that you'd be the one who knows for sure), or adding something like "u32 nextseq = PPP_CB(tail)->sequence + 1;" around line 2795, and then using "ppp->nextseq = nextseq;" on 2840, then I'd be in favor of that, at least to make the tool happy.

Is there an equivalent of "/* LINTED: tail pointer still ok */" here?

Signature · 1 line

James Carlson         42.703N 71.076W         <[email protected]>

Dan Carpenter

author

2021-08-02 11:43 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 11 lines · James Carlson

Quoted 5 lines
Quoted 2 lines
    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.
This is where things break down. That queue walker macro on line 2798 re-assigns 'p'. The code marches over the list and says "anything that still exists up to (but not including) the head for this completed packet is trash." Note that *NOTHING* here is harming 'head' or anything in the list that follows that buffer -- which includes 'tail.'

Crud... I can't believe I misread this code twice.

I'm not actually sure why Smatch doesn't get this correct. I wanted to blame it on the new unpublished bits but even the published code is buggy. :/ I will investigate and fix this.

Thanks for taking the time on this.

regards,
dan carpenter

Dan Carpenter

author

2021-08-02 12:37 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 10 lines · James Carlson

Quoted 8 lines · James Carlson
Quoted 4 lines · Dan Carpenter
Quoted 2 lines
--> 2840 		ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is where Smatch complains.
If that's Smatch's analysis of the situation, then Smatch is wrong. It's a bogus warning.

Quoted 8 lines

For what it's worth, it's not my code, and I agree that it's at least a bit hard to follow, and may well harbor bugs. If you're suggesting either some kind of suppression directive (I tried looking for some Smatch documentation but couldn't find much to suggest that's possible, though I guess now that you'd be the one who knows for sure), or adding something like "u32 nextseq = PPP_CB(tail)->sequence + 1;" around line 2795, and then using "ppp->nextseq = nextseq;" on 2840, then I'd be in favor of that, at least to make the tool happy.

No. No. Never change the code just to make the tool happy. Of course, I misread this in two different ways because the first time I didn't spot the break statement and the second time I got skb_queue_walk_safe() and skb_queue_walk_from_safe(). But it's not really hard to read if you're more familiar with those macros.

I've investigated this and it turns out the problem is a kind of known issue with how Smatch parses lists. I've avoided fixing this for years because parsing lists correctly will be a big slow down and it's a quite a big project but probably I should just fix it. Maybe later this year.

regards,
dan carpenter