On Sat, 1 Aug 2020 18:52:27 +0300
Vitaliy Makkoveev <m...@openbsd.org> wrote:
> On Sat, Aug 01, 2020 at 07:44:17PM +0900, YASUOKA Masahiko wrote:
>> I'm not sure when it is broken, in old versions, it was assumed the
>> pipex queues are empty when pipex_iface_stop() is called. The problem
>> mvs@ found is the assumption is not true any more.
>>
>> pipex has a mechanism that delete a session when the queues are empty.
>>
>> 819 Static void
>> 820 pipex_timer(void *ignored_arg)
>> 821 {
>> (snip)
>> 854 case PIPEX_STATE_CLOSED:
>> 855 /*
>> 856 * mbuf queued in pipexinq or pipexoutq may
>> have a
>> 857 * refererce to this session.
>> 858 */
>> 859 if (!mq_empty(&pipexinq) ||
>> !mq_empty(&pipexoutq))
>> 860 continue;
>> 861
>> 862 pipex_destroy_session(session);
>> 863 break;
>>
>> I think using this is better.
>>
>> How about this?
>
> Unfortunately your diff is incorrect. It introduces memory leaks and
> breaks pppx(4). Also it is incomplete.
Thank you for your feedbacks.
> We have multiple ways to kill pipex(sessions):
>
> 1. pppx(4)
>
> We have `struct pppx_if' which has pointer to corresponding session and
> this session is accessed directly within pppx(4) layer. Since we can't
> destroy `ppp_if' in pipex(4) layer we can't destroy these sessions by
> pipex_timer(). The only way to destroy them is pppx_if_destroy() which:
>
> 1. unlink session by pipex_unlink_session()
> 2. detach corresponding `ifnet' by if_detach()
> 3. release session by pipex_rele_session()
>
> It's unsafe because mbuf queues can have references to this session.
Yes.
> 2. pppac(4)
>
> We have no direct access to corresponding sessions within pppac(4)
> layer. Also there are multiple ways to do this:
>
> 1. pipex_ioctl() with `PIPEXSMODE' command. Underlay pipex_iface_stop()
> walks through `pipex_session_list' and destroy sessions by
> pipex_destroy_session() call. It's unsafe because we don't check queues.
>
> 2. pipex_ioctl() with `PIPEXDSESSION'. pipex_close_session() will change
> session's state and pipex_timer() will kill this sessions later. This
> is the only safe way.
>
> 3. pipex_iface_fini(). The same as `PIPEXSMODE', pipex_iface_stop()
> kills sessions, Which is also unsafe. Also we have another use after
> free issue:
>
> ---- cut begin ----
>
> pipex_iface_fini(struct pipex_iface_context *pipex_iface)
> {
> pool_put(&pipex_session_pool, pipex_iface->multicast_session);
> NET_LOCK();
> pipex_iface_stop(pipex_iface);
> NET_UNLOCK();
> }
>
> ---- cut end ----
>
> `multicast_session' should be protected too. It also can be pushed to
> `pipexoutq'.
Yes, I missed this point.
> Also since this time pipexintr() and pipex_iface_fini() are
> both serialized by KERNEL_LOCK() too we can't destroy `multicast_session'
> which is in use by pipexintr(). But when we will drop KERNEL_LOCK()
> around pipexintr() we can catch use after free issue here. I already did
> diff for move this pool_put() under NET_LOCK(), but it was rejectedi by
> mpi@ because:
>
> ---- cut begin ----
> pipex_iface_fini() should be called on the last reference of the
>
> descriptor. So this shouldn't be necessary. If there's an issue
>
> with the current order of the operations, we should certainly fix
>
> it differently.
> ---- cut end ----
Yes, I understand what mpi@ is saying. But this is a separate story.
> So I repeat it again: npppd(8) can be killed in every moment by SIGKILL
> or by SIGSEGV and pppacclose() will be called and it will call
> pipex_iface_fini(). `multicast_session' can be used in this moment by
> pipexintr().
>
> And no locks protect `multicast_session' itself.
>
> The two diffs I proposed in this thread solve problems caused by
> pipexintr().
There are a lot of ways to solve the problems.
The diff I sent few days ago is to destruct the pipex sessions in the
pipex timer. As you pointed out it has some problems. Those problems
can be fixed, but I'd suggest another way. I attached at last.
The problem exposed is "use-after-free". Since I think this is not a
problem of parallel processing, having reference counter seems too
much for me.
The diff is not to refer the session by a pointer, but by the id.
The idea is come from IPsec tdb.
Comments?
diff --git a/sys/net/pipex.c b/sys/net/pipex.c
index 2ad7757fee9..5f24381878c 100644
--- a/sys/net/pipex.c
+++ b/sys/net/pipex.c
@@ -148,6 +148,7 @@ void
pipex_iface_init(struct pipex_iface_context *pipex_iface, u_int ifindex)
{
struct pipex_session *session;
+ struct pipex_hash_head *chain;
pipex_iface->pipexmode = 0;
pipex_iface->ifindex = ifindex;
@@ -168,7 +169,12 @@ pipex_iface_init(struct pipex_iface_context *pipex_iface,
u_int ifindex)
session->is_multicast = 1;
session->pipex_iface = pipex_iface;
session->ifindex = ifindex;
+ /* register it to the pipex id hash */
+ session->protocol = PIPEX_PROTO_NONE;
+ session->session_id = ifindex;
pipex_iface->multicast_session = session;
+ chain = PIPEX_ID_HASHTABLE(pipex_iface->multicast_session->session_id);
+ LIST_INSERT_HEAD(chain, pipex_iface->multicast_session, id_chain);
}
Static void
@@ -197,10 +203,11 @@ pipex_iface_stop(struct pipex_iface_context *pipex_iface)
void
pipex_iface_fini(struct pipex_iface_context *pipex_iface)
{
- pool_put(&pipex_session_pool, pipex_iface->multicast_session);
NET_LOCK();
+ LIST_REMOVE(pipex_iface->multicast_session, id_chain);
pipex_iface_stop(pipex_iface);
NET_UNLOCK();
+ pool_put(&pipex_session_pool, pipex_iface->multicast_session);
}
int
@@ -734,11 +741,14 @@ pipexintr(void)
u_int16_t proto;
struct mbuf *m;
struct mbuf_list ml;
+ uintptr_t cookie;
/* ppp output */
mq_delist(&pipexoutq, &ml);
while ((m = ml_dequeue(&ml)) != NULL) {
- pkt_session = m->m_pkthdr.ph_cookie;
+ cookie = (uintptr_t)m->m_pkthdr.ph_cookie;
+ pkt_session = pipex_lookup_by_session_id(
+ cookie >> 16, cookie & 0xffff);
if (pkt_session == NULL) {
m_freem(m);
continue;
@@ -775,7 +785,9 @@ pipexintr(void)
/* ppp input */
mq_delist(&pipexinq, &ml);
while ((m = ml_dequeue(&ml)) != NULL) {
- pkt_session = m->m_pkthdr.ph_cookie;
+ cookie = (uintptr_t)m->m_pkthdr.ph_cookie;
+ pkt_session = pipex_lookup_by_session_id(
+ cookie >> 16, cookie & 0xffff);
if (pkt_session == NULL) {
m_freem(m);
continue;
@@ -788,7 +800,10 @@ Static int
pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session,
struct mbuf_queue *mq)
{
- m0->m_pkthdr.ph_cookie = session;
+ uintptr_t cookie;
+
+ cookie = session->protocol << 16 | session->session_id;
+ m0->m_pkthdr.ph_cookie = (caddr_t)cookie;
/* XXX need to support other protocols */
m0->m_pkthdr.ph_ppp_proto = PPP_IP;
diff --git a/sys/net/pipex.h b/sys/net/pipex.h
index bba010b46aa..3f493f33c9a 100644
--- a/sys/net/pipex.h
+++ b/sys/net/pipex.h
@@ -44,6 +44,7 @@
{ "outq", CTLTYPE_NODE }, \
}
+#define PIPEX_PROTO_NONE 0
#define PIPEX_PROTO_L2TP 1 /* protocol L2TP */
#define PIPEX_PROTO_PPTP 2 /* protocol PPTP */
#define PIPEX_PROTO_PPPOE 3 /* protocol PPPoE */
