Re: Prevent memory corruption by pipex_timer()
Hi, Sorry for my silence. ok yasuoka for the daemon part. On Wed, 1 Apr 2020 09:27:10 +0200 Martin Pieuchot wrote: > On 31/03/20(Tue) 23:16, Vitaliy Makkoveev wrote: >> On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote: >> > [...] >> > Well better fix npppd(8), no? Not crashing the kernel is priority 1. >> I made patch for npppd(8) too. I include it to this email below, without >> starting new thread, ok? If ioctl(PIPEXASESSION) failed and error != >> ENXIO it means that pipex is enabled and session creation failed so down >> this connection. > > Thanks, I committed the kernel part. I'm waiting to see if other devs > want to comment on the daemon part. > >> > Then if the daemon has a bug, should the kernel work around it? >> In most common cases should :( > > That's an opinion. There's no true or false answer. Working around it > has obvious advantages but it doesn't make us fix existing bug and instead > force us to maintain the work around. > > It is argued that the "failing hard" model, as it is practised in OpenBSD > software development, has the advantage of resulting in simpler code because > every layer is responsible for handling errors and doesn't pile workaround. > > This bug is a nice example. Thanks for the report! If you could submit > your refactoring in a new thread, I'd love to look at it. >
Re: Prevent memory corruption by pipex_timer()
On 31/03/20(Tue) 23:16, Vitaliy Makkoveev wrote: > On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote: > > [...] > > Well better fix npppd(8), no? Not crashing the kernel is priority 1. > I made patch for npppd(8) too. I include it to this email below, without > starting new thread, ok? If ioctl(PIPEXASESSION) failed and error != > ENXIO it means that pipex is enabled and session creation failed so down > this connection. Thanks, I committed the kernel part. I'm waiting to see if other devs want to comment on the daemon part. > > Then if the daemon has a bug, should the kernel work around it? > In most common cases should :( That's an opinion. There's no true or false answer. Working around it has obvious advantages but it doesn't make us fix existing bug and instead force us to maintain the work around. It is argued that the "failing hard" model, as it is practised in OpenBSD software development, has the advantage of resulting in simpler code because every layer is responsible for handling errors and doesn't pile workaround. This bug is a nice example. Thanks for the report! If you could submit your refactoring in a new thread, I'd love to look at it.
Re: Prevent memory corruption by pipex_timer()
On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote:
> On 31/03/20(Tue) 18:58, Vitaliy Makkoveev wrote:
> > On Tue, Mar 31, 2020 at 05:23:46PM +0200, Martin Pieuchot wrote:
> > > On 31/03/20(Tue) 16:48, Vitaliy Makkoveev wrote:
> > > > I refactored pppx(4). The idea is to use pipex(4) as it was designed.
> > > > No one holds pipex_session outside pipex(4) so pipex_timer() calls are
> > > > safe. Unfortunately, this way gives some performance impact, because we
> > > > need to call pipex_lookup_by_session_id() in some places. This impact
> > > > will gone after pipex_session becames safely shared between multiple
> > > > holders and this is my next goal.
> > >
> > > I'd be more confident if we could go with the one-line change that you
> > > submitted in the first mail of this thread without the debugging
> > > messages.
> > >
> > > Mixing bug-fixes (or features) and refactoring is not a great
> > > development practise as it might hide the point of the change and
> > > introduce or expose new bugs.
> > >
> > I changed my original patch. Since npppd(8) ignores ioctl() errors and
> > client will be connected without pppx(4) interface creation I decide to
> > lie npppd(8).
>
> Well better fix npppd(8), no? Not crashing the kernel is priority 1.
I made patch for npppd(8) too. I include it to this email below, without
starting new thread, ok? If ioctl(PIPEXASESSION) failed and error !=
ENXIO it means that pipex is enabled and session creation failed so down
this connection.
> Then if the daemon has a bug, should the kernel work around it?
In most common cases should :(
cut begin
Index: usr.sbin/npppd/npppd/ppp.c
===
RCS file: /cvs/src/usr.sbin/npppd/npppd/ppp.c,v
retrieving revision 1.28
diff -u -p -r1.28 ppp.c
--- usr.sbin/npppd/npppd/ppp.c 27 Feb 2019 04:52:19 - 1.28
+++ usr.sbin/npppd/npppd/ppp.c 31 Mar 2020 19:59:21 -
@@ -1134,8 +1134,12 @@ ppp_on_network_pipex(npppd_ppp *_this)
(!MPPE_MUST_NEGO(_this) || _this->ccp.fsm.state == OPENED ||
_this->ccp.fsm.state == STOPPED)) {
/* IPCP is opened and MPPE is not required or MPPE is opened */
- if (npppd_ppp_pipex_enable(_this->pppd, _this) != 0)
+ if (npppd_ppp_pipex_enable(_this->pppd, _this) != 0) {
ppp_log(_this, LOG_WARNING, "failed enable pipex: %m");
+ /* failed to create pipex session */
+ ppp_phy_downed(_this);
+ return;
+ }
ppp_log(_this, LOG_NOTICE, "Using pipex=%s",
(_this->pipex_enabled != 0)? "yes" : "no");
_this->pipex_started = 1;
cut end
And patch for pppx()
cut begin
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ sys/net/if_pppx.c 31 Mar 2020 20:00:33 -
@@ -665,6 +665,12 @@ pppx_add_session(struct pppx_dev *pxd, s
struct ifnet *over_ifp = NULL;
#endif
+ /*
+* XXX: prevent pxi destruction by pipex_timer()
+*/
+ if (req->pr_timeout_sec != 0)
+ return (EINVAL);
+
switch (req->pr_protocol) {
#ifdef PIPEX_PPPOE
case PIPEX_PROTO_PPPOE:
cut end
Re: Prevent memory corruption by pipex_timer()
On 31/03/20(Tue) 18:58, Vitaliy Makkoveev wrote:
> On Tue, Mar 31, 2020 at 05:23:46PM +0200, Martin Pieuchot wrote:
> > On 31/03/20(Tue) 16:48, Vitaliy Makkoveev wrote:
> > > I refactored pppx(4). The idea is to use pipex(4) as it was designed.
> > > No one holds pipex_session outside pipex(4) so pipex_timer() calls are
> > > safe. Unfortunately, this way gives some performance impact, because we
> > > need to call pipex_lookup_by_session_id() in some places. This impact
> > > will gone after pipex_session becames safely shared between multiple
> > > holders and this is my next goal.
> >
> > I'd be more confident if we could go with the one-line change that you
> > submitted in the first mail of this thread without the debugging
> > messages.
> >
> > Mixing bug-fixes (or features) and refactoring is not a great
> > development practise as it might hide the point of the change and
> > introduce or expose new bugs.
> >
> I changed my original patch. Since npppd(8) ignores ioctl() errors and
> client will be connected without pppx(4) interface creation I decide to
> lie npppd(8).
Well better fix npppd(8), no? Not crashing the kernel is priority 1.
Then if the daemon has a bug, should the kernel work around it?
> Index: net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 if_pppx.c
> --- net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
> +++ net/if_pppx.c 31 Mar 2020 15:48:26 -
> @@ -665,6 +665,13 @@ pppx_add_session(struct pppx_dev *pxd, s
> struct ifnet *over_ifp = NULL;
> #endif
>
> + /*
> + * XXX: prevent pxi destruction by pipex_timer()
> + * npppd(8) ignores pppx_add_session() errors so fake it
> + */
> + if (req->pr_timeout_sec != 0)
> + req->pr_timeout_sec = 0;
> +
> switch (req->pr_protocol) {
> #ifdef PIPEX_PPPOE
> case PIPEX_PROTO_PPPOE:
>
Re: Prevent memory corruption by pipex_timer()
On Tue, Mar 31, 2020 at 05:23:46PM +0200, Martin Pieuchot wrote:
> On 31/03/20(Tue) 16:48, Vitaliy Makkoveev wrote:
> > I refactored pppx(4). The idea is to use pipex(4) as it was designed.
> > No one holds pipex_session outside pipex(4) so pipex_timer() calls are
> > safe. Unfortunately, this way gives some performance impact, because we
> > need to call pipex_lookup_by_session_id() in some places. This impact
> > will gone after pipex_session becames safely shared between multiple
> > holders and this is my next goal.
>
> I'd be more confident if we could go with the one-line change that you
> submitted in the first mail of this thread without the debugging
> messages.
>
> Mixing bug-fixes (or features) and refactoring is not a great
> development practise as it might hide the point of the change and
> introduce or expose new bugs.
>
I changed my original patch. Since npppd(8) ignores ioctl() errors and
client will be connected without pppx(4) interface creation I decide to
lie npppd(8).
Index: net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ net/if_pppx.c 31 Mar 2020 15:48:26 -
@@ -665,6 +665,13 @@ pppx_add_session(struct pppx_dev *pxd, s
struct ifnet *over_ifp = NULL;
#endif
+ /*
+* XXX: prevent pxi destruction by pipex_timer()
+* npppd(8) ignores pppx_add_session() errors so fake it
+*/
+ if (req->pr_timeout_sec != 0)
+ req->pr_timeout_sec = 0;
+
switch (req->pr_protocol) {
#ifdef PIPEX_PPPOE
case PIPEX_PROTO_PPPOE:
Re: Prevent memory corruption by pipex_timer()
On 31/03/20(Tue) 16:48, Vitaliy Makkoveev wrote: > I refactored pppx(4). The idea is to use pipex(4) as it was designed. > No one holds pipex_session outside pipex(4) so pipex_timer() calls are > safe. Unfortunately, this way gives some performance impact, because we > need to call pipex_lookup_by_session_id() in some places. This impact > will gone after pipex_session becames safely shared between multiple > holders and this is my next goal. I'd be more confident if we could go with the one-line change that you submitted in the first mail of this thread without the debugging messages. Mixing bug-fixes (or features) and refactoring is not a great development practise as it might hide the point of the change and introduce or expose new bugs.
Re: Prevent memory corruption by pipex_timer()
I refactored pppx(4). The idea is to use pipex(4) as it was designed.
No one holds pipex_session outside pipex(4) so pipex_timer() calls are
safe. Unfortunately, this way gives some performance impact, because we
need to call pipex_lookup_by_session_id() in some places. This impact
will gone after pipex_session becames safely shared between multiple
holders and this is my next goal.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ sys/net/if_pppx.c 31 Mar 2020 13:28:54 -
@@ -155,8 +155,8 @@ struct pppx_if {
int pxi_unit;
struct ifnetpxi_if;
struct pppx_dev *pxi_dev;
- struct pipex_sessionpxi_session;
struct pipex_iface_context pxi_ifcontext;
+ int pxi_ppp_id;
};
static inline int
@@ -178,7 +178,8 @@ int pppx_del_session(struct pppx_dev *,
intpppx_set_session_descr(struct pppx_dev *,
struct pipex_session_descr_req *);
-void pppx_if_destroy(struct pppx_dev *, struct pppx_if *);
+intpppx_if_destroy(struct pppx_dev *, struct pppx_if *,
+ struct pipex_session_close_req *);
void pppx_if_start(struct ifnet *);
intpppx_if_output(struct ifnet *, struct mbuf *,
struct sockaddr *, struct rtentry *);
@@ -587,7 +588,7 @@ pppxclose(dev_t dev, int flags, int mode
/* XXX */
NET_LOCK();
while ((pxi = LIST_FIRST(>pxd_pxis)))
- pppx_if_destroy(pxd, pxi);
+ pppx_if_destroy(pxd, pxi, NULL);
NET_UNLOCK();
LIST_REMOVE(pxd, pxd_entry);
@@ -655,160 +656,14 @@ int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
struct pppx_if *pxi;
- struct pipex_session *session;
- struct pipex_hash_head *chain;
struct ifnet *ifp;
int unit, error = 0;
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
-#ifdef PIPEX_PPPOE
- struct ifnet *over_ifp = NULL;
-#endif
-
- switch (req->pr_protocol) {
-#ifdef PIPEX_PPPOE
- case PIPEX_PROTO_PPPOE:
- over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
- if (over_ifp == NULL)
- return (EINVAL);
- if (req->pr_peer_address.ss_family != AF_UNSPEC)
- return (EINVAL);
- break;
-#endif
-#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
- case PIPEX_PROTO_PPTP:
- case PIPEX_PROTO_L2TP:
- switch (req->pr_peer_address.ss_family) {
- case AF_INET:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
- return (EINVAL);
- break;
-#ifdef INET6
- case AF_INET6:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in6))
- return (EINVAL);
- break;
-#endif
- default:
- return (EPROTONOSUPPORT);
- }
- if (req->pr_peer_address.ss_family !=
- req->pr_local_address.ss_family ||
- req->pr_peer_address.ss_len !=
- req->pr_local_address.ss_len)
- return (EINVAL);
- break;
-#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
- default:
- return (EPROTONOSUPPORT);
- }
pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
- if (pxi == NULL)
- return (ENOMEM);
-
- session = >pxi_session;
ifp = >pxi_if;
- /* fake a pipex interface context */
- session->pipex_iface = >pxi_ifcontext;
- session->pipex_iface->ifnet_this = ifp;
- session->pipex_iface->pipexmode = PIPEX_ENABLED;
-
- /* setup session */
- session->state = PIPEX_STATE_OPENED;
- session->protocol = req->pr_protocol;
- session->session_id = req->pr_session_id;
- session->peer_session_id = req->pr_peer_session_id;
- session->peer_mru = req->pr_peer_mru;
- session->timeout_sec = req->pr_timeout_sec;
- session->ppp_flags = req->pr_ppp_flags;
- session->ppp_id = req->pr_ppp_id;
-
- session->ip_forward = 1;
-
- session->ip_address.sin_family = AF_INET;
- session->ip_address.sin_len = sizeof(struct sockaddr_in);
- session->ip_address.sin_addr = req->pr_ip_address;
-
- session->ip_netmask.sin_family = AF_INET;
- session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
- session->ip_netmask.sin_addr = req->pr_ip_netmask;
-
- if (session->ip_netmask.sin_addr.s_addr == 0L)
- session->ip_netmask.sin_addr.s_addr =
Re: Prevent memory corruption by pipex_timer()
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.
Re: Prevent memory corruption by pipex_timer()
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. 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.
Re: Prevent memory corruption by pipex_timer()
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.
cut begin
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(void)
{
int fd;
struct pipex_session_req req;
if((fd=open("/dev/pppx0", O_RDWR))<0){
err(1, "open()");
}
memset(, 0, sizeof(req));
req.pr_timeout_sec=0;
req.pr_protocol=PIPEX_PROTO_L2TP;
req.pr_local_address.ss_family=AF_INET;
req.pr_local_address.ss_len=sizeof(struct sockaddr_in);
req.pr_peer_address.ss_family=AF_INET;
req.pr_peer_address.ss_len=sizeof(struct sockaddr_in);
if(ioctl(fd, PIPEXASESSION, )<0){
err(1, "ioctl()");
}
select(0, NULL, NULL, NULL, NULL);
return 0;
}
cut end
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ sys/net/if_pppx.c 27 Mar 2020 12:02:33 -
@@ -665,6 +665,12 @@ pppx_add_session(struct pppx_dev *pxd, s
struct ifnet *over_ifp = NULL;
#endif
+#if 0
+ /* XXX: prevent pxi destruction by pipex_timer() */
+ if (req->pr_timeout_sec != 0)
+ return (EINVAL);
+#endif
+
switch (req->pr_protocol) {
#ifdef PIPEX_PPPOE
case PIPEX_PROTO_PPPOE:
@@ -706,6 +712,11 @@ pppx_add_session(struct pppx_dev *pxd, s
pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
if (pxi == NULL)
return (ENOMEM);
+
+#if 1
+ printf("%s: new pppx_if pipex_session %p timeout %u\n",
+ __func__, >pxi_session, req->pr_timeout_sec);
+#endif
session = >pxi_session;
ifp = >pxi_if;
Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.109
diff -u -p -r1.109 pipex.c
--- sys/net/pipex.c 26 Mar 2020 16:50:46 - 1.109
+++ sys/net/pipex.c 27 Mar 2020 12:02:34 -
@@ -767,6 +767,10 @@ pipex_timer(void *ignored_arg)
session->stat.idle_time++;
if (session->stat.idle_time < session->timeout_sec)
continue;
+#if 1
+ printf("%s: pipex_session %p timeout\n",
+ __func__, session);
+#endif
pipex_notify_close_session(session);
break;
@@ -792,6 +796,10 @@ pipex_timer(void *ignored_arg)
continue;
pipex_destroy_session(session);
+#if 1
+ printf("%s: pipex_session %p killed\n",
+ __func__, session);
+#endif
break;
default:
Re: Prevent memory corruption by pipex_timer()
On Fri, Mar 27, 2020 at 03:16:54PM +0300, 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.
>
> cut begin
>
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> #include
> #include
> #include
> #include
>
> int main(void)
> {
> int fd;
> struct pipex_session_req req;
>
> if((fd=open("/dev/pppx0", O_RDWR))<0){
> err(1, "open()");
> }
>
> memset(, 0, sizeof(req));
>
> req.pr_timeout_sec=0;
Sorry, this line should be "req.pr_timeout_sec=1;"
>
> req.pr_protocol=PIPEX_PROTO_L2TP;
> req.pr_local_address.ss_family=AF_INET;
> req.pr_local_address.ss_len=sizeof(struct sockaddr_in);
> req.pr_peer_address.ss_family=AF_INET;
> req.pr_peer_address.ss_len=sizeof(struct sockaddr_in);
>
> if(ioctl(fd, PIPEXASESSION, )<0){
> err(1, "ioctl()");
> }
>
> select(0, NULL, NULL, NULL, NULL);
>
> return 0;
> }
>
> cut end
>
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 if_pppx.c
> --- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
> +++ sys/net/if_pppx.c 27 Mar 2020 12:02:33 -
> @@ -665,6 +665,12 @@ pppx_add_session(struct pppx_dev *pxd, s
> struct ifnet *over_ifp = NULL;
> #endif
>
> +#if 0
> + /* XXX: prevent pxi destruction by pipex_timer() */
> + if (req->pr_timeout_sec != 0)
> + return (EINVAL);
> +#endif
> +
> switch (req->pr_protocol) {
> #ifdef PIPEX_PPPOE
> case PIPEX_PROTO_PPPOE:
> @@ -706,6 +712,11 @@ pppx_add_session(struct pppx_dev *pxd, s
> pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> if (pxi == NULL)
> return (ENOMEM);
> +
> +#if 1
> + printf("%s: new pppx_if pipex_session %p timeout %u\n",
> + __func__, >pxi_session, req->pr_timeout_sec);
> +#endif
>
> session = >pxi_session;
> ifp = >pxi_if;
> Index: sys/net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 pipex.c
> --- sys/net/pipex.c 26 Mar 2020 16:50:46 - 1.109
> +++ sys/net/pipex.c 27 Mar 2020 12:02:34 -
> @@ -767,6 +767,10 @@ pipex_timer(void *ignored_arg)
> session->stat.idle_time++;
> if (session->stat.idle_time < session->timeout_sec)
> continue;
> +#if 1
> + printf("%s: pipex_session %p timeout\n",
> + __func__, session);
> +#endif
>
> pipex_notify_close_session(session);
> break;
> @@ -792,6 +796,10 @@ pipex_timer(void *ignored_arg)
> continue;
>
> pipex_destroy_session(session);
> +#if 1
> + printf("%s: pipex_session %p killed\n",
> + __func__, session);
> +#endif
> break;
>
> default:
Re: Prevent memory corruption by pipex_timer()
On 27/03/20(Fri) 11:58, Vitaliy Makkoveev wrote:
> Each pipex_session has timeout_sec field and if it is not 0,
> pipex_timer() can destroy pipex_session. Each pppx_if contents
> pipex_session. If userland creates pppx_if and pipex_session has
> timeout (for example, npppd.conf has idle-timeout option), pipex_timer()
> can destroy this pipex_session and break it's pppx_if. Diff below is
> quick and dirty fix for this case. At first look, creation of
> pipex_timer()-like pppx_timer() is more better, but I am little confused
> with pipexintr(). Looks like pipexoutq and pipexinq can contain mbufs
> with holds pointer to already destructed pipex_session, because
> pipex_destroy_session() and pppx_if_destroy() just frees pipex_session
> and potential pppx_timer() is only anoter point to crash kernel.
Do you have a backtrace for the memory corruption? Could you share it?
> I suggest, pipex_session and pppx_if should be refactored for good fix,
> at least:
> 1. add reference counter for pipex_session
> 2. pppx_if should hold a reference to pipex_session
> 3. pipex_session should have handler to inform it's holder about
> destruction
>
> Default npppd.conf has no idle-timeout option, so npppd will not be very
> confused with this diff.
>
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 if_pppx.c
> --- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
> +++ sys/net/if_pppx.c 27 Mar 2020 08:20:50 -
> @@ -665,6 +665,10 @@ pppx_add_session(struct pppx_dev *pxd, s
> struct ifnet *over_ifp = NULL;
> #endif
>
> + /* XXX: prevent pxi destruction by pipex_timer() */
> + if (req->pr_timeout_sec != 0)
> + return (EINVAL);
> +
> switch (req->pr_protocol) {
> #ifdef PIPEX_PPPOE
> case PIPEX_PROTO_PPPOE:
>
Prevent memory corruption by pipex_timer()
Each pipex_session has timeout_sec field and if it is not 0,
pipex_timer() can destroy pipex_session. Each pppx_if contents
pipex_session. If userland creates pppx_if and pipex_session has
timeout (for example, npppd.conf has idle-timeout option), pipex_timer()
can destroy this pipex_session and break it's pppx_if. Diff below is
quick and dirty fix for this case. At first look, creation of
pipex_timer()-like pppx_timer() is more better, but I am little confused
with pipexintr(). Looks like pipexoutq and pipexinq can contain mbufs
with holds pointer to already destructed pipex_session, because
pipex_destroy_session() and pppx_if_destroy() just frees pipex_session
and potential pppx_timer() is only anoter point to crash kernel.
I suggest, pipex_session and pppx_if should be refactored for good fix,
at least:
1. add reference counter for pipex_session
2. pppx_if should hold a reference to pipex_session
3. pipex_session should have handler to inform it's holder about
destruction
Default npppd.conf has no idle-timeout option, so npppd will not be very
confused with this diff.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ sys/net/if_pppx.c 27 Mar 2020 08:20:50 -
@@ -665,6 +665,10 @@ pppx_add_session(struct pppx_dev *pxd, s
struct ifnet *over_ifp = NULL;
#endif
+ /* XXX: prevent pxi destruction by pipex_timer() */
+ if (req->pr_timeout_sec != 0)
+ return (EINVAL);
+
switch (req->pr_protocol) {
#ifdef PIPEX_PPPOE
case PIPEX_PROTO_PPPOE:
