Re: vmm(4): unterminated vm_name after strncpy
On Sat, Mar 28, 2020 at 06:47:47PM -0600, Theo de Raadt wrote: > Or strncpy with length - 1 would be also good, since it won't copy >foo\0bar\0 > fully, but only >foo\0 > into the buffer and store it as >foo\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0 > and gaurantee the \0 on the in-kernel buffer. Agree, this sound even better. Index: vmm.c === RCS file: /cvs/src/sys/arch/amd64/amd64/vmm.c,v retrieving revision 1.268 diff -u -p -r1.268 vmm.c --- vmm.c 16 Mar 2020 08:21:16 - 1.268 +++ vmm.c 29 Mar 2020 00:52:05 - @@ -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); + strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN - 1); rw_enter_write(_softc->vm_lock);
Re: vmm(4): unterminated vm_name after strncpy
Or strncpy with length - 1 would be also good, since it won't copy foo\0bar\0 fully, but only foo\0 into the buffer and store it as foo\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0 and gaurantee the \0 on the in-kernel buffer.
Re: vmm(4): unterminated vm_name after strncpy
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 - 1.268 +++ sys/arch/amd64/amd64/vmm.c 29 Mar 2020 00:18:43 - @@ -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(_softc->vm_lock);
Re: vmm(4): unterminated vm_name after strncpy
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. 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. Greg Steuck wrote: > > Index: sys/arch/amd64/amd64/vmm.c > > === > > RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/amd64/vmm.c,v > > retrieving revision 1.266 > > diff -u -p -r1.266 vmm.c > > --- sys/arch/amd64/amd64/vmm.c 11 Mar 2020 16:38:42 - 1.266 > > +++ sys/arch/amd64/amd64/vmm.c 12 Mar 2020 21:15:01 - > > @@ -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; > > - strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN); > > + strlcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN); > > Coincidentally syzkaller managed to trigger a uvm_fault in this line. > https://syzkaller.appspot.com/bug?extid=48e38ebd31c030b5841c > > ddb> trace > strlcpy(80001d374448,80a2cc20,40) at strlcpy+0xcf > sys/lib/libkern/strlcpy.c:44 > vm_create(80a2c800,80001d339758) at vm_create+0x112 > sys/arch/amd64/amd64/vmm.c:1172 > VOP_IOCTL(fd805d843820,c5005601,80a2c800,1,fd806c3bfc00,80001d339758) > at VOP_IOCTL+0x88 sys/kern/vfs_vops.c:290 > vn_ioctl(fd805d877800,c5005601,80a2c800,80001d339758) at > vn_ioctl+0xb5 sys/kern/vfs_vnops.c:531 > > Unfortunately there's no reproducer > > Thanks > Greg > -- > nest.cx is Gmail hosted, use PGP: > https://pgp.key-server.io/0x0B1542BD8DF5A1B0 > Fingerprint: 5E2B 2D0E 1E03 2046 BEC3 4D50 0B15 42BD 8DF5 A1B0
Re: vmm(4): unterminated vm_name after strncpy
> Index: sys/arch/amd64/amd64/vmm.c > === > RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/amd64/vmm.c,v > retrieving revision 1.266 > diff -u -p -r1.266 vmm.c > --- sys/arch/amd64/amd64/vmm.c 11 Mar 2020 16:38:42 - 1.266 > +++ sys/arch/amd64/amd64/vmm.c 12 Mar 2020 21:15:01 - > @@ -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; > - strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN); > + strlcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN); Coincidentally syzkaller managed to trigger a uvm_fault in this line. https://syzkaller.appspot.com/bug?extid=48e38ebd31c030b5841c ddb> trace strlcpy(80001d374448,80a2cc20,40) at strlcpy+0xcf sys/lib/libkern/strlcpy.c:44 vm_create(80a2c800,80001d339758) at vm_create+0x112 sys/arch/amd64/amd64/vmm.c:1172 VOP_IOCTL(fd805d843820,c5005601,80a2c800,1,fd806c3bfc00,80001d339758) at VOP_IOCTL+0x88 sys/kern/vfs_vops.c:290 vn_ioctl(fd805d877800,c5005601,80a2c800,80001d339758) at vn_ioctl+0xb5 sys/kern/vfs_vnops.c:531 Unfortunately there's no reproducer Thanks Greg -- nest.cx is Gmail hosted, use PGP: https://pgp.key-server.io/0x0B1542BD8DF5A1B0 Fingerprint: 5E2B 2D0E 1E03 2046 BEC3 4D50 0B15 42BD 8DF5 A1B0
Re: vmm(4): unterminated vm_name after strncpy
On Thu, Mar 12, 2020 at 10:31:13PM +0100, Tobias Heider wrote:
> vmm uses 'strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN)' to copy
> to buffers of size VMM_MAX_NAME_LEN, which can leave the resulting string
> unterminated.
> From strncpy(3):
> strncpy() only NUL terminates the destination string when the length of
> the source string is less than the length parameter.
>
> I propose replacing it with 'strlcpy' which does the right thing and
> only copies up to dstsize - 1 characters.
>
> ok?
>
good find. Thanks!
> CID 1453255
>
> Index: sys/arch/amd64/amd64/vmm.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> retrieving revision 1.266
> diff -u -p -r1.266 vmm.c
> --- sys/arch/amd64/amd64/vmm.c11 Mar 2020 16:38:42 - 1.266
> +++ sys/arch/amd64/amd64/vmm.c12 Mar 2020 21:15:01 -
> @@ -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;
> - strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN);
> + strlcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN);
>
> rw_enter_write(_softc->vm_lock);
>
> @@ -3718,7 +3718,7 @@ vm_get_info(struct vm_info_params *vip)
> out[i].vir_ncpus = vm->vm_vcpu_ct;
> out[i].vir_id = vm->vm_id;
> out[i].vir_creator_pid = vm->vm_creator_pid;
> - strncpy(out[i].vir_name, vm->vm_name, VMM_MAX_NAME_LEN);
> + strlcpy(out[i].vir_name, vm->vm_name, VMM_MAX_NAME_LEN);
> rw_enter_read(>vm_vcpu_lock);
> for (j = 0; j < vm->vm_vcpu_ct; j++) {
> out[i].vir_vcpu_state[j] = VCPU_STATE_UNKNOWN;
>
Re: vmm(4): unterminated vm_name after strncpy
OK.
It appears this is already handled safely in vmd and vmctl -- the string
always had a terminal NUL before copying in any direction, and since both
arrays are the same storage size, there is no way to lose the NUL, and
the struct being filled is allocated with PR_ZERO.
Tobias Heider wrote:
> vmm uses 'strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN)' to copy
> to buffers of size VMM_MAX_NAME_LEN, which can leave the resulting string
> unterminated.
> From strncpy(3):
> strncpy() only NUL terminates the destination string when the length of
> the source string is less than the length parameter.
>
> I propose replacing it with 'strlcpy' which does the right thing and
> only copies up to dstsize - 1 characters.
>
> ok?
>
> CID 1453255
>
> Index: sys/arch/amd64/amd64/vmm.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> retrieving revision 1.266
> diff -u -p -r1.266 vmm.c
> --- sys/arch/amd64/amd64/vmm.c11 Mar 2020 16:38:42 - 1.266
> +++ sys/arch/amd64/amd64/vmm.c12 Mar 2020 21:15:01 -
> @@ -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;
> - strncpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN);
> + strlcpy(vm->vm_name, vcp->vcp_name, VMM_MAX_NAME_LEN);
>
> rw_enter_write(_softc->vm_lock);
>
> @@ -3718,7 +3718,7 @@ vm_get_info(struct vm_info_params *vip)
> out[i].vir_ncpus = vm->vm_vcpu_ct;
> out[i].vir_id = vm->vm_id;
> out[i].vir_creator_pid = vm->vm_creator_pid;
> - strncpy(out[i].vir_name, vm->vm_name, VMM_MAX_NAME_LEN);
> + strlcpy(out[i].vir_name, vm->vm_name, VMM_MAX_NAME_LEN);
> rw_enter_read(>vm_vcpu_lock);
> for (j = 0; j < vm->vm_vcpu_ct; j++) {
> out[i].vir_vcpu_state[j] = VCPU_STATE_UNKNOWN;
>
