From 5cb8094031249a31d1b4c37cf708ed72395e9043 Mon Sep 17 00:00:00 2001 From: Justus Winter <4winter@informatik.uni-hamburg.de> Date: Tue, 18 Aug 2015 11:32:15 +0200 Subject: [PATCH gnumach 2/2] ipc: fix locking issues * ipc/ipc_port.c (ipc_port_destroy): Avoid accessing `port's fields without the lock. (ipc_port_alloc_special): Lock `port'. * ipc/mach_msg.c (mach_msg_trap): Avoid using `ipc_port_flag_protected_payload' on unlocked port. * ipc/ipc_kmsg.c (ipc_kmsg_copyout_header): Likewise. --- ipc/ipc_kmsg.c | 16 ++++++++++++---- ipc/ipc_port.c | 17 ++++++++++------- ipc/mach_msg.c | 16 +++++++++++++--- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c index 5076809..6c4b59a 100644 --- a/ipc/ipc_kmsg.c +++ b/ipc/ipc_kmsg.c @@ -1713,6 +1713,7 @@ ipc_kmsg_copyout_header( mach_port_t dest_name; ipc_port_t nsrequest; unsigned long payload; + int have_payload; /* receiving an asynchronous message */ @@ -1732,6 +1733,7 @@ ipc_kmsg_copyout_header( else dest_name = MACH_PORT_NULL; payload = dest->ip_protected_payload; + have_payload = ipc_port_flag_protected_payload(dest); if ((--dest->ip_srights == 0) && ((nsrequest = dest->ip_nsrequest) != IP_NULL)) { @@ -1745,7 +1747,7 @@ ipc_kmsg_copyout_header( } else ip_unlock(dest); - if (! ipc_port_flag_protected_payload(dest)) { + if (! have_payload) { msg->msgh_bits = (MACH_MSGH_BITS_OTHER(mbits) | MACH_MSGH_BITS(0, MACH_MSG_TYPE_PORT_SEND)); msg->msgh_local_port = dest_name; @@ -1766,6 +1768,7 @@ ipc_kmsg_copyout_header( mach_port_t dest_name, reply_name; ipc_port_t nsrequest; unsigned long payload; + int have_payload; /* receiving a request message */ @@ -1837,6 +1840,7 @@ ipc_kmsg_copyout_header( else dest_name = MACH_PORT_NULL; payload = dest->ip_protected_payload; + have_payload = ipc_port_flag_protected_payload(dest); if ((--dest->ip_srights == 0) && ((nsrequest = dest->ip_nsrequest) != IP_NULL)) { @@ -1850,7 +1854,7 @@ ipc_kmsg_copyout_header( } else ip_unlock(dest); - if (! ipc_port_flag_protected_payload(dest)) { + if (! have_payload) { msg->msgh_bits = (MACH_MSGH_BITS_OTHER(mbits) | MACH_MSGH_BITS(MACH_MSG_TYPE_PORT_SEND_ONCE, MACH_MSG_TYPE_PORT_SEND)); @@ -1868,6 +1872,7 @@ ipc_kmsg_copyout_header( case MACH_MSGH_BITS(MACH_MSG_TYPE_PORT_SEND_ONCE, 0): { mach_port_t dest_name; unsigned long payload; + int have_payload; /* receiving a reply message */ @@ -1882,6 +1887,7 @@ ipc_kmsg_copyout_header( assert(dest->ip_sorights > 0); payload = dest->ip_protected_payload; + have_payload = ipc_port_flag_protected_payload(dest); if (dest->ip_receiver == space) { ip_release(dest); @@ -1895,7 +1901,7 @@ ipc_kmsg_copyout_header( dest_name = MACH_PORT_NULL; } - if (! ipc_port_flag_protected_payload(dest)) { + if (! have_payload) { msg->msgh_bits = (MACH_MSGH_BITS_OTHER(mbits) | MACH_MSGH_BITS(0, MACH_MSG_TYPE_PORT_SEND_ONCE)); @@ -1922,6 +1928,7 @@ ipc_kmsg_copyout_header( ipc_port_t reply = (ipc_port_t) msg->msgh_local_port; mach_port_t dest_name, reply_name; unsigned long payload; + int have_payload; if (IP_VALID(reply)) { ipc_port_t notify_port; @@ -2161,6 +2168,7 @@ ipc_kmsg_copyout_header( copyout_dest: payload = dest->ip_protected_payload; + have_payload = ipc_port_flag_protected_payload(dest); if (ip_active(dest)) { ipc_object_copyout_dest(space, (ipc_object_t) dest, @@ -2189,7 +2197,7 @@ ipc_kmsg_copyout_header( if (IP_VALID(reply)) ipc_port_release(reply); - if (! ipc_port_flag_protected_payload(dest)) { + if (! have_payload) { msg->msgh_bits = (MACH_MSGH_BITS_OTHER(mbits) | MACH_MSGH_BITS(reply_type, dest_type)); msg->msgh_local_port = dest_name; diff --git a/ipc/ipc_port.c b/ipc/ipc_port.c index 86a4ee2..dd8d92c 100644 --- a/ipc/ipc_port.c +++ b/ipc/ipc_port.c @@ -635,6 +635,7 @@ ipc_port_destroy( ipc_kmsg_t kmsg; ipc_thread_t sender; ipc_port_request_t dnrequests; + struct ipc_target *ip_target; assert(ip_active(port)); /* port->ip_receiver_name is garbage */ @@ -694,17 +695,20 @@ ipc_port_destroy( port->ip_object.io_bits &= ~IO_BITS_ACTIVE; port->ip_timestamp = ipc_port_timestamp(); + + nsrequest = port->ip_nsrequest; + mqueue = &port->ip_messages; + dnrequests = port->ip_dnrequests; + ip_target = &port->ip_target; + ip_unlock(port); /* throw away no-senders request */ - - nsrequest = port->ip_nsrequest; if (nsrequest != IP_NULL) ipc_notify_send_once(nsrequest); /* consumes ref */ /* destroy any queued messages */ - mqueue = &port->ip_messages; imq_lock(mqueue); assert(ipc_thread_queue_empty(&mqueue->imq_threads)); kmqueue = &mqueue->imq_messages; @@ -725,8 +729,6 @@ ipc_port_destroy( imq_unlock(mqueue); /* generate dead-name notifications */ - - dnrequests = port->ip_dnrequests; if (dnrequests != IPR_NULL) { ipc_table_size_t its = dnrequests->ipr_size; ipc_table_elems_t size = its->its_size; @@ -753,7 +755,7 @@ ipc_port_destroy( ipc_kobject_destroy(port); /* Common destruction for the IPC target. */ - ipc_target_terminate(&port->ip_target); + ipc_target_terminate(ip_target); ipc_port_release(port); /* consume caller's ref */ } @@ -1183,6 +1185,7 @@ ipc_port_alloc_special(ipc_space_t space) return IP_NULL; ip_lock_init(port); + ip_lock(port); port->ip_references = 1; port->ip_object.io_bits = io_makebits(TRUE, IOT_PORT, 0); @@ -1198,7 +1201,7 @@ ipc_port_alloc_special(ipc_space_t space) */ ipc_port_init(port, space, (mach_port_t)port); - + ip_unlock(port); return port; } diff --git a/ipc/mach_msg.c b/ipc/mach_msg.c index fe0c43e..371d725 100644 --- a/ipc/mach_msg.c +++ b/ipc/mach_msg.c @@ -954,6 +954,7 @@ mach_msg_trap( (ipc_port_t) kmsg->ikm_header.msgh_local_port; mach_port_t dest_name, reply_name; unsigned long payload; + int have_payload; /* receiving a request message */ @@ -1018,6 +1019,8 @@ mach_msg_trap( else dest_name = MACH_PORT_NULL; payload = dest_port->ip_protected_payload; + have_payload = + ipc_port_flag_protected_payload(dest_port); if ((--dest_port->ip_srights == 0) && (dest_port->ip_nsrequest != IP_NULL)) { @@ -1035,7 +1038,7 @@ mach_msg_trap( } else ip_unlock(dest_port); - if (! ipc_port_flag_protected_payload(dest_port)) { + if (! have_payload) { kmsg->ikm_header.msgh_bits = MACH_MSGH_BITS( MACH_MSG_TYPE_PORT_SEND_ONCE, MACH_MSG_TYPE_PORT_SEND); @@ -1059,6 +1062,7 @@ mach_msg_trap( case MACH_MSGH_BITS(MACH_MSG_TYPE_PORT_SEND_ONCE, 0): { mach_port_t dest_name; unsigned long payload; + int have_payload; /* receiving a reply message */ @@ -1071,6 +1075,8 @@ mach_msg_trap( assert(dest_port->ip_sorights > 0); payload = dest_port->ip_protected_payload; + have_payload = + ipc_port_flag_protected_payload(dest_port); if (dest_port->ip_receiver == space) { ip_release(dest_port); @@ -1084,7 +1090,7 @@ mach_msg_trap( dest_name = MACH_PORT_NULL; } - if (! ipc_port_flag_protected_payload(dest_port)) { + if (! have_payload) { kmsg->ikm_header.msgh_bits = MACH_MSGH_BITS( 0, MACH_MSG_TYPE_PORT_SEND_ONCE); @@ -1104,6 +1110,7 @@ mach_msg_trap( MACH_MSGH_BITS(MACH_MSG_TYPE_PORT_SEND_ONCE, 0): { mach_port_t dest_name; unsigned long payload; + int have_payload; /* receiving a complex reply message */ @@ -1116,6 +1123,9 @@ mach_msg_trap( assert(dest_port->ip_sorights > 0); payload = dest_port->ip_protected_payload; + have_payload = + ipc_port_flag_protected_payload(dest_port); + if (dest_port->ip_receiver == space) { ip_release(dest_port); @@ -1129,7 +1139,7 @@ mach_msg_trap( dest_name = MACH_PORT_NULL; } - if (! ipc_port_flag_protected_payload(dest_port)) { + if (! have_payload) { kmsg->ikm_header.msgh_bits = MACH_MSGH_BITS_COMPLEX | MACH_MSGH_BITS( -- 2.1.4