Date: Thu, 9 Feb 2006 19:22:38 GMT From: Rob Deker <deker@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 91450 for review Message-ID: <200602091922.k19JMcF7028657@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=91450 Change 91450 by deker@deker_build1.columbia.sparta.com on 2006/02/09 19:21:49 This combines 2 2 changes. Both comments from millert included: "Fix the order of the message checks; we need to do the port check before the rights check (it was the other way around). To do this we must Move mac_check_port_send() out of ipc_kmsg_send() and into ipc_kmsg_copyin(). It is too late to deny in ipc_kmsg_send() since the rights have already have been copied at this point." "Move the mac_check_port_send() call down into ipc_kmsg_copyin_header(). This reduces the amount of duplicated setup we have to do. Change an assert into an if() since apparently we can end up with a NULL ipc_port_t when the system is rebooting." Submitted by: millert Affected files ... .. //depot/projects/trustedbsd/sedarwin7/src/darwin/xnu/osfmk/ipc/ipc_kmsg.c#7 edit Differences ... ==== //depot/projects/trustedbsd/sedarwin7/src/darwin/xnu/osfmk/ipc/ipc_kmsg.c#7 (text+ko) ==== @@ -1024,6 +1024,9 @@ ipc_object_t dest_port, reply_port; ipc_port_t dest_soright, reply_soright; ipc_port_t notify_port; +#ifdef MAC + ipc_entry_t entry; +#endif if ((mbits != msg->msgh_bits) || (!MACH_MSG_TYPE_PORT_ANY_SEND(dest_type)) || @@ -1041,6 +1044,33 @@ if (!MACH_PORT_VALID(dest_name)) goto invalid_dest; +#ifdef MAC + /* + * We do the port send check here instead of in ipc_kmsg_send() + * because copying the header involves copying the port rights too + * and we need to do the send check before anything is actually copied. + */ + entry = ipc_entry_lookup(space, dest_name); + if (entry != IE_NULL) { + int error = 0; + ipc_port_t port = (ipc_port_t) entry->ie_object; + // assert(port != IP_NULL); + if (port == IP_NULL) + goto invalid_dest; + ip_lock(port); + if (ip_active(port)) { + task_t self = current_task(); + tasklabel_lock(self); + error = mac_check_port_send(&self->maclabel, + &port->ip_label); + tasklabel_unlock(self); + } + ip_unlock(port); + if (error != 0) + goto invalid_dest; + } +#endif + if (notify != MACH_PORT_NULL) { ipc_entry_t entry; @@ -1718,45 +1748,6 @@ mach_port_name_t notify) { mach_msg_return_t mr; -#ifdef MAC - mach_port_name_t dest_name; - ipc_entry_t entry; - ipc_port_t port; - task_t self; - int error = 0; - - /* - * We do the port send check here instead of in ipc_kmsg_send() - * because copying the header involves copying the port rights too - * and we need to do the send check before anything is actually copied. - * We don't currently try to mediate kernel-resident servers. - */ - self = current_task(); - if (self != kernel_task) { - is_read_lock(space); - if (space->is_active) { - dest_name = (mach_port_name_t) kmsg->ikm_header.msgh_remote_port; - if (MACH_PORT_VALID(dest_name)) { - entry = ipc_entry_lookup(space, dest_name); - if (entry != IE_NULL) { - port = (ipc_port_t) entry->ie_object; - assert(port != IP_NULL); - ip_lock(port); - if (ip_active(port)) { - tasklabel_lock(self); - error = mac_check_port_send(&self->maclabel, - &port->ip_label); - tasklabel_unlock(self); - } - ip_unlock(port); - } - } - } - is_read_unlock(space); - if (error != 0) - return MACH_SEND_INVALID_DEST; - } -#endif mr = ipc_kmsg_copyin_header(&kmsg->ikm_header, space, notify); if (mr != MACH_MSG_SUCCESS) @@ -1993,6 +1984,7 @@ ipc_port_request_index_t request; if (!space->is_active) { + printf("ipc_kmsg_copyout_header: dead space\n"); is_write_unlock(space); return (MACH_RCV_HEADER_ERROR| MACH_MSG_IPC_SPACE); @@ -2002,6 +1994,7 @@ notify_port = ipc_port_lookup_notify(space, notify); if (notify_port == IP_NULL) { + printf("ipc_kmsg_copyout_header: no notify port\n"); is_write_unlock(space); return MACH_RCV_INVALID_NOTIFY; } @@ -2056,13 +2049,16 @@ if (kr != KERN_SUCCESS) { /* space is unlocked */ - if (kr == KERN_RESOURCE_SHORTAGE) + if (kr == KERN_RESOURCE_SHORTAGE) { + printf("ipc_kmsg_copyout_header: can't grow kernel ipc space\n"); return (MACH_RCV_HEADER_ERROR| MACH_MSG_IPC_KERNEL); - else + } else { + printf("ipc_kmsg_copyout_header: can't grow user ipc space\n"); return (MACH_RCV_HEADER_ERROR| MACH_MSG_IPC_SPACE); } + } /* space is locked again; start over */ continue; @@ -2099,9 +2095,11 @@ kr = ipc_port_dngrow(reply, ITS_SIZE_NONE); /* port is unlocked */ - if (kr != KERN_SUCCESS) + if (kr != KERN_SUCCESS) { + printf("ipc_kmsg_copyout_header: can't grow kernel ipc space2\n"); return (MACH_RCV_HEADER_ERROR| MACH_MSG_IPC_KERNEL); + } is_write_lock(space); continue; @@ -2121,6 +2119,7 @@ kr = ipc_right_copyout(space, reply_name, entry, reply_type, TRUE, (ipc_object_t) reply); /* reply port is unlocked */ + /* XXXMAC - ipc_right_copyout() *can* fail due to MAC */ assert(kr == KERN_SUCCESS); if (notify_port != IP_NULL) @@ -2138,6 +2137,7 @@ is_read_lock(space); if (!space->is_active) { + printf("ipc_kmsg_copyout_header: dead space2\n"); is_read_unlock(space); return MACH_RCV_HEADER_ERROR|MACH_MSG_IPC_SPACE; } @@ -2148,11 +2148,13 @@ /* must check notify even though it won't be used */ if ((entry = ipc_entry_lookup(space, notify)) == IE_NULL) { + printf("ipc_kmsg_copyout_header: ipc_entry_lookup failed\n"); is_read_unlock(space); return MACH_RCV_INVALID_NOTIFY; } if ((entry->ie_bits & MACH_PORT_TYPE_RECEIVE) == 0) { + printf("ipc_kmsg_copyout_header: MACH_PORT_TYPE_RECEIVE not set!\n"); is_read_unlock(space); return MACH_RCV_INVALID_NOTIFY; } @@ -2537,8 +2539,10 @@ mach_msg_return_t mr; mr = ipc_kmsg_copyout_header(&kmsg->ikm_header, space, notify); - if (mr != MACH_MSG_SUCCESS) + if (mr != MACH_MSG_SUCCESS) { + printf("ipc_kmsg_copyout: ipc_kmsg_copyout_header failed: %d\n", mr); return mr; + } if (kmsg->ikm_header.msgh_bits & MACH_MSGH_BITS_COMPLEX) { mr = ipc_kmsg_copyout_body(kmsg, space, map, slist);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200602091922.k19JMcF7028657>