[PATCH 2/2] net: tftp: Prevent too early device removal leading to data aborts

[Miquel Raynal]

All network related commands are executed through a "net loop", which
handles the initialization, setup and removal of all the required
resources to perform the operation.

Aside from the generic network boilerplate, the net loop calls specific
helpers provided by the different command implementations. The tftp
implementation, among others, registers his own handler with
net_set_udp_handler() in order to catch incoming UDP packets. In this
handler, there are several calls to eth_halt(). Some of them might be
legitimate, but one that is certainly not is the one in the switch case
handling the "access denied" and "file not found errors".

When these error happens, there is nothing wrong with the network
interface but the protocol itself returned an error condition. There is
no reason to immediately halt the Ethernet interface, because the
handler will propagate an error code which the net loop will process, up
to the point where it will halt and remove the Ethernet interface
itself. Said otherwise, halting the Ethernet interface (and at the same
time removing all structures related to it) in the tftp handler bypasses
the net loop.

This would not cause any harm if halting the interface would not
sometimes lead to its entire removal. That is indeed what happens with
the Ethernet USB gadget implementation. In this driver, the ->start()
and ->stop() callbacks are responsible for creating and removing a
virtual Ethernet interface, which means after the ->stop() call, any
structure used to define that interface shall no longer be used. The
eth_halt() call stopping (and removing) the interface in the handler,
would make the net loop perform illegal accesses over freed buffers,
overwriting some content in the heap. These overwrites have been
identified to later lead to serious data aborts within malloc()
execution.

Let's just drop this eth_alt() call which is happening too early, and
is redundant anyway.

Suggested-by: Qianfan Zhao <qianfanguijin at 163.com>
Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
---
 net/tftp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tftp.c b/net/tftp.c
index 88e71e67de3..ac20a60d222 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -680,7 +680,6 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		case TFTP_ERR_FILE_NOT_FOUND:
 		case TFTP_ERR_ACCESS_DENIED:
 			puts("Not retrying...\n");
-			eth_halt();
 			net_set_state(NETLOOP_FAIL);
 			break;
 		case TFTP_ERR_UNDEFINED:
-- 
2.34.1