On Fri, Mar 27, 2020 at 03:13:01PM +0100, Martin Pieuchot wrote:
> On 27/03/20(Fri) 15:16, Vitaliy Makkoveev wrote:
> > On Fri, Mar 27, 2020 at 10:43:52AM +0100, Martin Pieuchot wrote:
> > > Do you have a backtrace for the memory corruption? Could you share it?
> > Yes. Apply path below, compile and run code, and when you had see
> > "pipex_session ... killed" kill this code. Screenshot attached.
> > STABLE-6.[56] are affected too.
>
> Thanks for the screenshot. The backtraces it contains is the most
> important piece of informations when reporting such bug.
>
> So we're faced with a double-free. After setting a timeout to a
> non-NULL value pipex_timer() will free the descriptor in
> pipex_destroy_session(), later when the program terminates and the
> pseudo-device is closed the same descriptor is being freed by
> pppx_if_destroy().
>
> But more importantly, pipex_destroy_session() puts a pointer back to a
> pool from which it hasn't been allocated (see line 597 of net/pipex.c).
>
> To fix this particular double free the question is: do you want to use
> this timer feature with pppx(4)? Does it even make sense? If not I
> guess your fix is the way to go.
I don't, but npppd(8) can. With my fix if npppd.conf has lines
tunnel L2TP protocol l2tp {
listen on 0.0.0.0
idle-timeout 1 # non-NULL value
}
npppd(8) can't create pppx interface. Without my fix npppd(8) can crash
kernel with this config. Does it make sense? npppd.conf(5) says:
"The default is 0, which disables the idle timer." Does anybody use
non-default value? I don't know, but there is no crash reports before.
>
> Due to the amount of code duplicated (copy-pasted) between net/pipex.c
> and net/if_pppx.c it is not unlikely that more of such bugs exist.
>
> So it would be nice to introduce some helper function like session_free()
> and session_setup() to make sure the same code, including safety checks,
> is run everywhere.
>
I suggest, use-after-free bug exists in pipexintr(). See lines 787-795
of net/pipex.c. Destruction of pipex_session denied if mbuf queues are
not empty. But pipex_destroy_session() call within pipex_iface_stop()
hasn't this check. Destruction by pppx_if_destroy() also hasn't this
check. But mbufs with m_pkthdr.ph_cookie pointed to freed session can
exist in those queues (lines 671 and 708 of net/pipex.c). I suggest it
should be fixed first.
