upap_timeout is not currently stopped on authentication success or
fail events. This may have strange results if session is restarted
in a high pace because even if the timeout callback have a sanity
check against the PAP state the session can be restarted and be
back in the valid state before the timeout callback is actually
called.
Do not assume LWIP_RAND will return 32 bits of randomness because it is
probably going to be defined to directly return the rand() value. For
example, LCP magic numbers are 32-bit random values.
This is already what we inherently have always done for IPv4/IPv6
packets, so it works. Receivers must handle both cases anyway because
both behaviors are seen in the wild.
A previous call to ppp_input might have disconnected the session while
there were still packets in flight in the tcpip mailbox. Drop incoming
packets because ppp_input must never be called if the upper layer is
down.
Speed-up a little bit the PPPoS input parser by only checking the open
flag after calling the ppp_input function, the only one that can induce
a state change here.
ppp_set_* functions that set the PPP session parameters must only be
called when the session is in a dead state (i.e. disconnected),
otherwise not fatal but surprising results may happen.
This function call the notify phase callback that should be called from
the lwIP core thread. This is especially true if the user callback
is not designed to be reentrant.
There is no good reason why this function should take a non-const
pointer. While changing that also switch to a more generic `void*`
instead of "byte".
There is no good reason why this function should take a non-const
pointer. While changing that also switch to a more generic `void*`
instead of "byte".
We do not have equivalents in PPPAPI for ppp_set_* functions because
calling them only makes sense while session is disconnected, furthermore
they are only setting structure members of the session configuration.
We only have to reserve header space for forwarding for IPv4 and IPv6
packets, all other packets are PPP control packets. Doing so reduce
the need of having to coalesce the PBUF chain before PPP processes
control packets.
PPP peer can negotiate its MRU, therefore we don't know the MTU we are
going to use before starting PPP. This is an issue because netif_add
function assume that the netif init callback function will set the MTU,
netif_add will then copy mtu to mtu6. We have then to update mtu6 each
time we update mtu to keep them in sync. Doing so is fine because PPP
netif MTU is only updated when the netif is in link down state.
Our current HDLC decoder does not protect against starving the Rx
PBUF POOL for one packet, most likely due to received garbage on
the serial port.
Prevent starving the Rx pool by checking incoming packets length
against PPP_MRU with a 10% margin because we only want to avoid
filling all PBUFs with garbage, we don't have to be pedantic.
Fixes bug #58441: Invalid PPP data accumulates forever.
PPP_MRU is now free to be used for what it should have been. Now using
it at PPP init stage to set the wanted MRU value, triggering a MRU
negotiation at the LCP phase.
I doubt anyone needs it anyway, but, well, at least it is fixed and the
MRU/MTU config mess is cleaned.
And while we are at it, better document PPP MRU config values.
Those are private functions, using the netif_ prefix here is not really
nice, especially with functions named netif_set_mtu and netif_get_mtu
for obvious reasons.
We currently retry indefinitely if sending packets fails, for example
if the output interface is down. We are even doing it if we are in
a middle of a connection process. This is not a very nice behavior
because PPP low level will retry indefinitely to connect and the user
application will never be warned that something is wrong.
We have the persist boolean in PPP settings to achieve more or less
the same thing anyway. Except it does it better at only retrying
indefinitely the initiation packet.
Having it configurable does not really make sense anymore, we already
need PBUF_RAM in all transmit paths. There are no real reason to keep
allocating PPP response buffers from the PBUF_POOL which should be now
reserved for receive paths only.
When pbuf_coalesce fails it does nothing and returns the previous buffer
chain. Adds checks that pbuf_coalesce succeeded, otherwise drop incoming
packet.
If we fail to receive a full packet, for exemple if a memory allocation
fail for some reason, we currently do not wait for next packet flag
character and we start filling a new packet at next received byte. Then
we expect the checksum check to discard the packet.
The behavior seem to have been broken one or two decades ago when adding
support for PFC (Protocol-Field-Compression) and ACFC
(Address-and-Control-Field-Compression).
Rework to drop any character until we receive a flag character at init
and when we drop a packet before it is complete.
In theory, if provided username or password is over 0x80000000 byte long
(err...), casts to signed integer of strlen() return values is going to
return negative values breaking lengths checks.
Fix it by only using unsigned integer or size_t (guaranteed to be
unsigned) comparisons.
User application code should be responsible to call netif_set_up() but
let's not break compatibility for now.
Signed-off-by: Sylvain Rochet <gradator@gradator.net>
NETIF_FLAG_UP flag is not supposed to be set by netif init callback
anymore, call netif_set_up() instead.
Sure it would be better to let user application code call netif_set_up()
by itself as it is now meant to be but let's not break compatibility for
now and add a FIXME for next release with allowed behavior break.
Signed-off-by: Sylvain Rochet <gradator@gradator.net>
This reverts commit 6e7ea92d56e43de65c46396b82ceebce2b95e8af.
We better forbid building configurations that does not make sense instead
of bloating the code with more ifdef. Here building CCP support without
adding any compressor support serve no real use case.
This adds some basic checks to the subroutines of eap_input to check
that we have requested or agreed to doing EAP authentication before
doing any processing on the received packet. The motivation is to
make it harder for a malicious peer to disrupt the operation of pppd
by sending unsolicited EAP packets. Note that eap_success() already
has a check that the EAP client state is reasonable, and does nothing
(apart from possibly printing a debug message) if not.
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Sylvain Rochet <gradator@gradator.net> (ported to lwIP PPP pcb struct)
Given that we have just checked vallen < len, it can never be the case
that vallen >= len + sizeof(rhostname). This fixes the check so we
actually avoid overflowing the rhostname array.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Sylvain Rochet <gradator@gradator.net> (compiler warning fix about int vs uint comparisons)
This converts all ppp_*() debug functions to ppp_*(()) macros that
ensure the code is left out by the linker if the corresponding debug
setting is disabled.
Downside is that many lines of code are touched, but since these
already differ to upstream PPP sources, I figured that's ok...
See bug #55199
Signed-off-by: Simon Goldschmidt <goldsimon@gmx.de>
Make pppoe_create() actually store the passed service name and
concentrator name, so that they are passed in the PADI/PADR/PADS
packets.
Assume that the user application won't be freeing the strings and just
copy the string pointers, therefore remove the mem_free() in
pppoe_destroy().
Since only the pointers are copied now, make them 'const' in
pppoe_softc.
Signed-off-by: Sylvain Rochet <gradator@gradator.net>
Rename PPPOE_TODO to PPPOE_SCNAME_SUPPORT because this is the only
feature enclosed by them. Prepare for proper service name and
concentrator name support by moving PPPOE_SCNAME_SUPPORT define to
ppp_opts.h.
Signed-off-by: Sylvain Rochet <gradator@gradator.net>
pppos_input() is safe to call from outside tcpip_thread when
PPP_INPROC_IRQ_SAFE == 1, so only check if PPP_INPROC_IRQ_SAFE == 0
Signed-off-by: Simon Goldschmidt <goldsimon@gmx.de>
