On Sat, Mar 28, 2020 at 05:33:05PM -0600, Theo de Raadt wrote:
> Pretty obvious why.
>
> The kernel doesn't check it's a string, before calling strlcpy
> which (correctly) runs off the array hunting for the terminal NUL,
> and into the next object, and I guess it finds a NUL in the next
> VA page which isn't actually mapped with storage.
Makes sense.
>
> With strncpy, this was safe. The input storage wasn't a real
> string, and the interior storage wasn't a real string either. Then
> on the output side, this was handled.
>
> There two choices to go with:
>
> - validate the input is a string before calling strlcpy.
> - copy the whole region with memcpy, and then manuall NUL-terminate
> the buffer.
>
> The same will apply to other vcp ioctl fields.
I couldn't find any other vcp field where this applies.
So here is a fix using memcpy instead. We don't even need to manually
NUL-terminate in this case because 'vm' is allocated with PR_ZERO.
Index: sys/arch/amd64/amd64/vmm.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/amd64/vmm.c,v
retrieving revision 1.268
diff -u -p -r1.268 vmm.c
--- sys/arch/amd64/amd64/vmm.c 16 Mar 2020 08:21:16 -0000 1.268
+++ sys/arch/amd64/amd64/vmm.c 29 Mar 2020 00:18:43 -0000
@@ -1167,7 +1167,7 @@ vm_create(struct vm_create_params *vcp,
memcpy(vm->vm_memranges, vcp->vcp_memranges,
vm->vm_nmemranges * sizeof(vm->vm_memranges[0]));
vm->vm_memory_size = memsize;
- strlcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN);
+ memcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN - 1);
rw_enter_write(&vmm_softc->vm_lock);
