[bug #58441] Invalid PPP data accumulates forever

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[bug #58441] Invalid PPP data accumulates forever

yuanjianmin
URL:
  <https://savannah.nongnu.org/bugs/?58441>

                 Summary: Invalid PPP data accumulates forever
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: ashesman
            Submitted on: Mon 25 May 2020 03:50:41 AM UTC
                Category: PPP
                Severity: 3 - Normal
              Item Group: Faulty Behaviour
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: 2.1.1

    _______________________________________________________

Details:

Today I had an issue with the wifi module I use where it crashes and resets,
dropping its baud rate in the process and dumping a whole lot of rubbish on
the serial port.  This is received as a long series of 0x80, 0x80, 0x80, x80,
... due to the baud rate mismatch.

I did a bit of debugging and noticed that this sequence of data just keeps
getting added to a pbuf forever by pppos_input, there are no checks that
result in the data being discarded.

Forgive me if this is just a side effect of the PPP protocol that I don't
understand that is unavoidable.  I am also unable to propose a suitable
solution due to my lack of PPP protocol knowledge.  The correct solution is to
discard the UART received bytes that were corrupt as UART would have received
them with an error flag.

There is a check at line 629 of pppos.c that is currently disabled.  This
check would have caught this situation.

        case PDCONTROL:                 /* Process control field. */
          /* If we don't get a valid control code, restart. */
          if (cur_char == PPP_UI) {
            pppos->in_state = PDPROTOCOL1;
            break;
          }
          /* no break */

#if 0
          else {
            PPPDEBUG(LOG_WARNING,
                     ("pppos_input[%d]: Invalid control <%d>\n",
ppp->netif->num, cur_char));
            pppos->in_state = PDSTART;
          }
#endif
          /* Fall through */

Below is some example PPP test data that shows what I received:

~!E<0x00><0x00>7<0x00>\n<0x00><0x00><0xff><0x11>ax<0xac><0x1e><0x00><0xfb><0xac><0x1e><0x00><0xfc><0x00><0x17><0x00><0x17><0x00>#<0xe3><0x82>\r\n+UUWLE:0,6038E0864CC0,5\r\n<0xa6><0xe3>~

<0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80><0x80>





    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?58441>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #58441] Invalid PPP data accumulates forever

yuanjianmin
Update of bug #58441 (project lwip):

                Severity:              3 - Normal => 4 - Important          
              Item Group:        Faulty Behaviour => Change Request        
                  Status:                    None => Confirmed              
             Assigned to:                    None => gradator              

    _______________________________________________________

Follow-up Comment #1:

Indeed, that can be troublesome for systems with multiple netifs as we do not
have separate Rx pools for interfaces, starving the Rx pool for garbage is not
a really nice behavior. For single netif system that should not be an annoying
problem since the worse that can happen is a failure to allocate the single
PBUF needed for PPPoS Tx when PPPoS Rx is currently filling the last Rx PBUF,
hoping it does not happen at a noticeable rate.

Looks like we need to add a PPPoS MRU (Maximum Receive Unit) size in build
time options to prevent that. Validating early the frame header could prevent
queuing up to the MRU size but by itself is not enough because a very valid
and very long PPP frame can still starve the Rx pool.

Sylvain


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?58441>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #58441] Invalid PPP data accumulates forever

yuanjianmin
Update of bug #58441 (project lwip):

                Severity:           4 - Important => 3 - Normal            
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #2:

Fixed in commit a39ce0f53b2a57c4e7fbb252efb579a629201868.

Thank you for the report !


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?58441>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #58441] Invalid PPP data accumulates forever

yuanjianmin
Follow-up Comment #3, bug #58441 (project lwip):

This issue was reported against v2.1.1, so I'm wondering if it should be
applied to STABLE-2_1_x branch as well.


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?58441>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #58441] Invalid PPP data accumulates forever

yuanjianmin
Follow-up Comment #4, bug #58441 (project lwip):

I agree that should be a good candidate, it is not that bad in the wild but
the fix is harmless. The branch does not seem to be updated that often, or at
least, not day to day, I have no clue if Simon keep a list somewhere or if he
just cherry-pick what seem to be fixes when he want to do a release.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?58441>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel