Applied, thanks! Flavio Cruz, le dim. 19 févr. 2023 18:06:51 -0500, a ecrit: > We introduce both a user alignment and a kernel alignment. These are > separate requirements since for 64 bit with a 32 bit kernel we need to > ensure the kernel can consume messages that are 8-byte aligned. This > change removes any possibility of undefined behavior and also allows the > kernel to support 64 bit RPCs for the userland. > > A lot of the code that performs alignment was simplified under the > assumption that the message headers are well aligned. To enforce that > going forward, a few static assertions were added. > --- > include/mach/message.h | 34 +++++++--- > ipc/ipc_kmsg.c | 144 ++++++++++++++++------------------------- > x86_64/copy_user.c | 36 +++++++---- > 3 files changed, 101 insertions(+), 113 deletions(-) > > diff --git a/include/mach/message.h b/include/mach/message.h > index eb3b34c0..22a17b03 100644 > --- a/include/mach/message.h > +++ b/include/mach/message.h > @@ -334,19 +334,33 @@ typedef integer_t mach_msg_option_t; > > #define MACH_SEND_ALWAYS 0x00010000 /* internal use only */ > > -/* This is the alignment of msg descriptors and the actual data. > +#ifdef KERNEL > +/* This is the alignment of msg descriptors and the actual data > + * for both in kernel messages and user land messages. > * > - * On x86 it is made equal to the default structure alignment on > - * 32-bit, so we can easily maintain compatibility with 32-bit user > - * space on a 64-bit kernel. Other architectures might have different > - * needs, so this value might change in the future for differents > - * architectures. > + * We have two types of alignment because for specific configurations > + * (in particular a 64 bit kernel with 32 bit userland) we transform > + * 4-byte aligned user messages into 8-byte aligned messages (and vice-versa) > + * so that kernel messages are correctly aligned. > */ > -#define MACH_MSG_ALIGNMENT 4 > +#define MACH_MSG_KERNEL_ALIGNMENT sizeof(uintptr_t) > +#ifdef __x86_64__ > +#ifdef USER32 > +#define MACH_MSG_USER_ALIGNMENT 4 > +#else > +#define MACH_MSG_USER_ALIGNMENT 8 > +#endif > +#else > +#define MACH_MSG_USER_ALIGNMENT 4 > +#endif > > -#define mach_msg_is_misaligned(x) ( ((vm_offset_t)(x)) & > (MACH_MSG_ALIGNMENT-1) ) > -#define mach_msg_align(x) \ > - ( ( ((vm_offset_t)(x)) + (MACH_MSG_ALIGNMENT-1) ) & > ~(MACH_MSG_ALIGNMENT-1) ) > +#define mach_msg_align(x, alignment) \ > + ( ( ((vm_offset_t)(x)) + ((alignment)-1) ) & ~((alignment)-1) ) > +#define mach_msg_user_align(x) mach_msg_align(x, MACH_MSG_USER_ALIGNMENT) > +#define mach_msg_kernel_align(x) mach_msg_align(x, MACH_MSG_KERNEL_ALIGNMENT) > +#define mach_msg_user_is_misaligned(x) ((x) & ((MACH_MSG_USER_ALIGNMENT)-1)) > +#define mach_msg_kernel_is_misaligned(x) ((x) & > ((MACH_MSG_KERNEL_ALIGNMENT)-1)) > +#endif /* KERNEL */ > > /* > * Much code assumes that mach_msg_return_t == kern_return_t. > diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c > index dac4f5dc..1988da45 100644 > --- a/ipc/ipc_kmsg.c > +++ b/ipc/ipc_kmsg.c > @@ -229,28 +229,23 @@ ipc_kmsg_clean_body( > type = (mach_msg_type_long_t *) saddr; > is_inline = ((mach_msg_type_t*)type)->msgt_inline; > if (((mach_msg_type_t*)type)->msgt_longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - saddr = mach_msg_align(saddr); > - continue; > - } > name = type->msgtl_name; > size = type->msgtl_size; > number = type->msgtl_number; > saddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > number = ((mach_msg_type_t*)type)->msgt_number; > saddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - saddr = mach_msg_align(saddr); > - > /* calculate length of data in bytes, rounding up */ > > length = ((number * size) + 7) >> 3; > @@ -283,9 +278,7 @@ ipc_kmsg_clean_body( > } > > if (is_inline) { > - /* inline data sizes round up to int boundaries */ > - > - saddr += (length + 3) &~ 3; > + saddr += length; > } else { > vm_offset_t data = * (vm_offset_t *) saddr; > > @@ -300,6 +293,7 @@ ipc_kmsg_clean_body( > > saddr += sizeof(vm_offset_t); > } > + saddr = mach_msg_kernel_align(saddr); > } > } > > @@ -387,31 +381,26 @@ ipc_kmsg_clean_partial( > boolean_t is_inline, is_port; > vm_size_t length; > > -xxx: type = (mach_msg_type_long_t *) eaddr; > + type = (mach_msg_type_long_t *) eaddr; > is_inline = ((mach_msg_type_t*)type)->msgt_inline; > if (((mach_msg_type_t*)type)->msgt_longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - eaddr = mach_msg_align(eaddr); > - goto xxx; > - } > name = type->msgtl_name; > size = type->msgtl_size; > rnumber = type->msgtl_number; > eaddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + eaddr = mach_msg_kernel_align(eaddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > rnumber = ((mach_msg_type_t*)type)->msgt_number; > eaddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + eaddr = mach_msg_kernel_align(eaddr); > + } > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - eaddr = mach_msg_align(eaddr); > - > /* calculate length of data in bytes, rounding up */ > > length = ((rnumber * size) + 7) >> 3; > @@ -501,7 +490,7 @@ ipc_kmsg_get( > { > ipc_kmsg_t kmsg; > > - if ((size < sizeof(mach_msg_user_header_t)) || (size & 3)) > + if ((size < sizeof(mach_msg_user_header_t)) || > mach_msg_user_is_misaligned(size)) > return MACH_SEND_MSG_TOO_SMALL; > > if (size <= IKM_SAVED_MSG_SIZE) { > @@ -553,7 +542,7 @@ ipc_kmsg_get_from_kernel( > ipc_kmsg_t kmsg; > > assert(size >= sizeof(mach_msg_header_t)); > - assert((size & 3) == 0); > + assert(!mach_msg_kernel_is_misaligned(size)); > > kmsg = ikm_alloc(size); > if (kmsg == IKM_NULL) > @@ -1307,6 +1296,10 @@ ipc_kmsg_copyin_body( > saddr = (vm_offset_t) (&kmsg->ikm_header + 1); > eaddr = (vm_offset_t) &kmsg->ikm_header + kmsg->ikm_header.msgh_size; > > + // We make assumptions about the alignment of the header. > + > _Static_assert(!mach_msg_kernel_is_misaligned(sizeof(mach_msg_header_t)), > + "mach_msg_header_t needs to be > MACH_MSG_KERNEL_ALIGNMENT aligned."); > + > while (saddr < eaddr) { > vm_offset_t taddr = saddr; > mach_msg_type_long_t *type; > @@ -1330,21 +1323,21 @@ ipc_kmsg_copyin_body( > is_inline = ((mach_msg_type_t*)type)->msgt_inline; > dealloc = ((mach_msg_type_t*)type)->msgt_deallocate; > if (longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - saddr = mach_msg_align(saddr); > - continue; > - } > name = type->msgtl_name; > size = type->msgtl_size; > number = type->msgtl_number; > saddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > number = ((mach_msg_type_t*)type)->msgt_number; > saddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } > > is_port = MACH_MSG_TYPE_PORT_ANY(name); > @@ -1359,21 +1352,13 @@ ipc_kmsg_copyin_body( > return MACH_SEND_INVALID_TYPE; > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - saddr = mach_msg_align(saddr); > - > /* calculate length of data in bytes, rounding up */ > > length = (((uint64_t) number * size) + 7) >> 3; > > if (is_inline) { > - vm_size_t amount; > - > - /* inline data sizes round up to int boundaries */ > + vm_size_t amount = length; > > - amount = (length + 3) &~ 3; > if ((eaddr - saddr) < amount) { > ipc_kmsg_clean_partial(kmsg, taddr, FALSE, 0); > return MACH_SEND_MSG_TOO_SMALL; > @@ -1384,9 +1369,6 @@ ipc_kmsg_copyin_body( > } else { > vm_offset_t addr; > > - if (MACH_MSG_ALIGNMENT > sizeof(mach_msg_type_t)) > - saddr = mach_msg_align(saddr); > - > if ((eaddr - saddr) < sizeof(vm_offset_t)) { > ipc_kmsg_clean_partial(kmsg, taddr, FALSE, 0); > return MACH_SEND_MSG_TOO_SMALL; > @@ -1491,6 +1473,7 @@ ipc_kmsg_copyin_body( > > complex = TRUE; > } > + saddr = mach_msg_kernel_align(saddr); > } > > if (!complex) > @@ -1613,28 +1596,23 @@ ipc_kmsg_copyin_from_kernel(ipc_kmsg_t kmsg) > longform = ((mach_msg_type_t*)type)->msgt_longform; > /* type->msgtl_header.msgt_deallocate not used */ > if (longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - saddr = mach_msg_align(saddr); > - continue; > - } > name = type->msgtl_name; > size = type->msgtl_size; > number = type->msgtl_number; > saddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > number = ((mach_msg_type_t*)type)->msgt_number; > saddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - saddr = mach_msg_align(saddr); > - > /* calculate length of data in bytes, rounding up */ > > length = ((number * size) + 7) >> 3; > @@ -1642,10 +1620,8 @@ ipc_kmsg_copyin_from_kernel(ipc_kmsg_t kmsg) > is_port = MACH_MSG_TYPE_PORT_ANY(name); > > if (is_inline) { > - /* inline data sizes round up to int boundaries */ > - > data = saddr; > - saddr += (length + 3) &~ 3; > + saddr += length; > } else { > /* > * The sender should supply ready-made memory > @@ -1682,6 +1658,7 @@ ipc_kmsg_copyin_from_kernel(ipc_kmsg_t kmsg) > MACH_MSGH_BITS_CIRCULAR; > } > } > + saddr = mach_msg_kernel_align(saddr); > } > } > > @@ -2386,28 +2363,23 @@ ipc_kmsg_copyout_body( > is_inline = ((mach_msg_type_t*)type)->msgt_inline; > longform = ((mach_msg_type_t*)type)->msgt_longform; > if (longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - saddr = mach_msg_align(saddr); > - continue; > - } > name = type->msgtl_name; > size = type->msgtl_size; > number = type->msgtl_number; > saddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > number = ((mach_msg_type_t*)type)->msgt_number; > saddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - saddr = mach_msg_align(saddr); > - > /* calculate length of data in bytes, rounding up */ > > length = (((uint64_t) number * size) + 7) >> 3; > @@ -2442,16 +2414,11 @@ ipc_kmsg_copyout_body( > } > > if (is_inline) { > - /* inline data sizes round up to int boundaries */ > - > ((mach_msg_type_t*)type)->msgt_deallocate = FALSE; > - saddr += (length + 3) &~ 3; > + saddr += length; > } else { > vm_offset_t data; > > - if (MACH_MSG_ALIGNMENT > sizeof(mach_msg_type_t)) > - saddr = mach_msg_align(saddr); > - > data = * (vm_offset_t *) saddr; > > /* copyout memory carried in the message */ > @@ -2502,6 +2469,9 @@ ipc_kmsg_copyout_body( > * (vm_offset_t *) saddr = addr; > saddr += sizeof(vm_offset_t); > } > + > + /* Next element is always correctly aligned */ > + saddr = mach_msg_kernel_align(saddr); > } > > return mr; > @@ -2827,21 +2797,21 @@ ipc_msg_print(mach_msg_header_t *msgh) > is_inline = ((mach_msg_type_t*)type)->msgt_inline; > dealloc = ((mach_msg_type_t*)type)->msgt_deallocate; > if (longform) { > - /* This must be aligned */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - (mach_msg_is_misaligned(type))) { > - saddr = mach_msg_align(saddr); > - continue; > - } > name = type->msgtl_name; > size = type->msgtl_size; > number = type->msgtl_number; > saddr += sizeof(mach_msg_type_long_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_long_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } else { > name = ((mach_msg_type_t*)type)->msgt_name; > size = ((mach_msg_type_t*)type)->msgt_size; > number = ((mach_msg_type_t*)type)->msgt_number; > saddr += sizeof(mach_msg_type_t); > + if > (mach_msg_kernel_is_misaligned(sizeof(mach_msg_type_t))) { > + saddr = mach_msg_kernel_align(saddr); > + } > } > > db_printf("-- type="); > @@ -2872,11 +2842,6 @@ ipc_msg_print(mach_msg_header_t *msgh) > return; > } > > - /* padding (ptrs and ports) ? */ > - if ((sizeof(natural_t) > sizeof(mach_msg_type_t)) && > - ((size >> 3) == sizeof(natural_t))) > - saddr = mach_msg_align(saddr); > - > /* calculate length of data in bytes, rounding up */ > > length = ((number * size) + 7) >> 3; > @@ -2885,7 +2850,7 @@ ipc_msg_print(mach_msg_header_t *msgh) > vm_size_t amount; > unsigned i, numwords; > > - /* inline data sizes round up to int boundaries */ > + /* round up to int boundaries for printing */ > amount = (length + 3) &~ 3; > if ((eaddr - saddr) < amount) { > db_printf("*** too small\n"); > @@ -2910,6 +2875,7 @@ ipc_msg_print(mach_msg_header_t *msgh) > db_printf("0x%x\n", * (vm_offset_t *) saddr); > saddr += sizeof(vm_offset_t); > } > + saddr = mach_msg_kernel_align(saddr); > } > } > #endif /* MACH_KDB */ > diff --git a/x86_64/copy_user.c b/x86_64/copy_user.c > index dd9fe2d7..089cb1d1 100644 > --- a/x86_64/copy_user.c > +++ b/x86_64/copy_user.c > @@ -136,7 +136,9 @@ size_t msg_usize(const mach_msg_header_t *kmsg) > boolean_t is_inline; > amount = unpack_msg_type(saddr, &name, &size, &number, &is_inline); > saddr += amount; > + saddr = mach_msg_kernel_align(saddr); > usize += amount; > + usize = mach_msg_user_align(usize); > > if (is_inline) > { > @@ -150,8 +152,6 @@ size_t msg_usize(const mach_msg_header_t *kmsg) > size_t n = descsize_to_bytes(size); > saddr += n*number; > usize += n*number; > - saddr = mach_msg_align(saddr); > - usize = mach_msg_align(usize); > } > } > else > @@ -160,6 +160,8 @@ size_t msg_usize(const mach_msg_header_t *kmsg) > saddr += sizeof(vm_offset_t); > usize += sizeof(rpc_vm_offset_t); > } > + saddr = mach_msg_kernel_align(saddr); > + usize = mach_msg_user_align(usize); > } > } > return usize; > @@ -168,7 +170,7 @@ size_t msg_usize(const mach_msg_header_t *kmsg) > /* > * Expand the msg header and, if required, the msg body (ports, pointers) > * > - * To not make the code too compicated, we use the fact that some fields of > + * To not make the code too complicated, we use the fact that some fields of > * mach_msg_header have the same size in the kernel and user variant > (basically > * all fields except ports and addresses) > */ > @@ -200,6 +202,10 @@ int copyinmsg (const void *userbuf, void *kernelbuf, > const size_t usize) > ksaddr = (vm_offset_t)(kmsg + 1); > usaddr = (vm_offset_t)(umsg + 1); > ueaddr = (vm_offset_t)umsg + usize; > + > + > _Static_assert(!mach_msg_user_is_misaligned(sizeof(mach_msg_user_header_t)), > + "mach_msg_user_header_t needs to be MACH_MSG_USER_ALIGNMENT > aligned."); > + > if (usize > sizeof(mach_msg_user_header_t)) > { > /* check we have at least space for an empty descryptor */ > @@ -210,8 +216,6 @@ int copyinmsg (const void *userbuf, void *kernelbuf, > const size_t usize) > mach_msg_type_size_t size; > mach_msg_type_number_t number; > boolean_t is_inline; > - usaddr = mach_msg_align(usaddr); > - ksaddr = mach_msg_align(ksaddr); > if (copyin_unpack_msg_type(usaddr, ksaddr, &name, &size, &number, > &is_inline, &amount)) > return 1; > @@ -220,7 +224,9 @@ int copyinmsg (const void *userbuf, void *kernelbuf, > const size_t usize) > // might need to adjust it later depending on the type > vm_offset_t ktaddr = ksaddr; > usaddr += amount; > + usaddr = mach_msg_user_align(usaddr); > ksaddr += amount; > + ksaddr = mach_msg_kernel_align(ksaddr); > > if (is_inline) > { > @@ -247,8 +253,6 @@ int copyinmsg (const void *userbuf, void *kernelbuf, > const size_t usize) > return 1; > usaddr += n*number; > ksaddr += n*number; > - usaddr = mach_msg_align(usaddr); > - ksaddr = mach_msg_align(ksaddr); > } > } > else > @@ -262,15 +266,19 @@ int copyinmsg (const void *userbuf, void *kernelbuf, > const size_t usize) > > if (copyin_address((rpc_vm_offset_t*)usaddr, > (vm_offset_t*)ksaddr)) > return 1; > - // advance one pointer > + // Advance one pointer. > ksaddr += sizeof(vm_offset_t); > usaddr += sizeof(rpc_vm_offset_t); > } > + // Note that we have to align because mach_port_name_t might not > align > + // with the required user alignment. > + usaddr = mach_msg_user_align(usaddr); > + ksaddr = mach_msg_kernel_align(ksaddr); > } > } > > kmsg->msgh_size = sizeof(mach_msg_header_t) + ksaddr - (vm_offset_t)(kmsg > + 1); > - kmsg->msgh_size = mach_msg_align(kmsg->msgh_size); > + kmsg->msgh_size = kmsg->msgh_size; > return 0; > } > > @@ -308,15 +316,15 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, > const size_t ksize) > mach_msg_type_size_t size; > mach_msg_type_number_t number; > boolean_t is_inline; > - usaddr = mach_msg_align(usaddr); > - ksaddr = mach_msg_align(ksaddr); > amount = unpack_msg_type(ksaddr, &name, &size, &number, > &is_inline); > // TODO: optimize and bring here type adjustment?? > vm_offset_t utaddr=usaddr, ktaddr=ksaddr; > if (copyout((void*)ksaddr, (void*)usaddr, amount)) > return 1; > usaddr += amount; > + usaddr = mach_msg_user_align(usaddr); > ksaddr += amount; > + ksaddr = mach_msg_kernel_align(ksaddr); > > if (is_inline) > { > @@ -341,8 +349,6 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, > const size_t ksize) > return 1; > usaddr += n*number; > ksaddr += n*number; > - usaddr = mach_msg_align(usaddr); > - ksaddr = mach_msg_align(ksaddr); > } > } > else > @@ -361,12 +367,14 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, > const size_t ksize) > ksaddr += sizeof(vm_offset_t); > usaddr += sizeof(rpc_vm_offset_t); > } > + usaddr = mach_msg_user_align(usaddr); > + ksaddr = mach_msg_kernel_align(ksaddr); > } > } > > mach_msg_size_t usize; > usize = sizeof(mach_msg_user_header_t) + usaddr - (vm_offset_t)(umsg + 1); > - usize = mach_msg_align(usize); > + usize = usize; > if (copyout(&usize, &umsg->msgh_size, sizeof(kmsg->msgh_size))) > return 1; > > -- > 2.39.1 > >
-- Samuel --- Pour une évaluation indépendante, transparente et rigoureuse ! Je soutiens la Commission d'Évaluation de l'Inria.