copystr(9): Move to deprecate (attempt #2)

cemeyer · cemeyer · commit 6aef78af13c3 · 2020-05-25T16:40:48.000Z
This reapplies logical r360944 and r360946 (reverting r360955), with fixed copystr() stand-in replacement macro. Eventually the goal is to convert consumers and kill the macro, but for a first step it helps if the macro is correct. Prior commit message: Unlike the other copy*() functions, it does not serve to copy from one address space to another or protect against potential faults. It's just an older incarnation of the now-more-common strlcpy(). Add a coccinelle script to tools/ which can be used to mechanically convert existing instances where replacement with strlcpy is trivial. In the two cases which matched, fuse_vfsops.c and union_vfsops.c, the code was further refactored manually to simplify. Replace the declaration of copystr() in systm.h with a small macro wrapper around strlcpy (with correction from brooks@ -- thanks). Remove N redundant MI implementations of copystr. For MIPS, this entailed inlining the assembler copystr into the only consumer, copyinstr, and making the latter a leaf function. Reviewed by: jhb (earlier version) Discussed with: brooks (thanks!) Differential Revision: https://reviews.freebsd.org/D24672
diff --git a/sys/amd64/amd64/support.S b/sys/amd64/amd64/support.S
@@ -1416,43 +1416,6 @@ copyinstr_toolong:
 	movl	$ENAMETOOLONG,%eax
 	jmp	cpystrflt_x
 
-/*
- * copystr(from, to, maxlen, int *lencopied)
- *         %rdi, %rsi, %rdx, %rcx
- */
-ENTRY(copystr)
-	PUSH_FRAME_POINTER
-	movq	%rdx,%r8			/* %r8 = maxlen */
-
-	incq    %rdx
-1:
-	decq	%rdx
-	jz	4f
-	movb	(%rdi),%al
-	movb	%al,(%rsi)
-	incq	%rsi
-	incq	%rdi
-	testb	%al,%al
-	jnz	1b
-
-	/* Success -- 0 byte reached */
-	decq	%rdx
-	xorl	%eax,%eax
-2:
-	testq	%rcx,%rcx
-	jz      3f
-	/* set *lencopied and return %rax */
-	subq	%rdx,%r8
-	movq	%r8,(%rcx)
-3:
-	POP_FRAME_POINTER
-	ret
-4:
-	/* rdx is zero -- return ENAMETOOLONG */
-	movl    $ENAMETOOLONG,%eax
-	jmp	2b
-END(copystr)
-
 /*
  * Handling of special amd64 registers and descriptor tables etc
  */
diff --git a/sys/arm/arm/copystr.S b/sys/arm/arm/copystr.S
@@ -60,39 +60,6 @@ __FBSDID("$FreeBSD$");
 	ldr	tmp, .Lpcb
 #endif
 
-/*
- * r0 - from
- * r1 - to
- * r2 - maxlens
- * r3 - lencopied
- *
- * Copy string from r0 to r1
- */
-ENTRY(copystr)
-	stmfd	sp!, {r4-r5}			/* stack is 8 byte aligned */
-	teq	r2, #0x00000000
-	mov	r5, #0x00000000
-	moveq	r0, #ENAMETOOLONG
-	beq	2f
-
-1:	ldrb	r4, [r0], #0x0001
-	add	r5, r5, #0x00000001
-	teq	r4, #0x00000000
-	strb	r4, [r1], #0x0001
-	teqne	r5, r2
-	bne	1b
-
-	teq	r4, #0x00000000
-	moveq	r0, #0x00000000
-	movne	r0, #ENAMETOOLONG
-
-2:	teq	r3, #0x00000000
-	strne	r5, [r3]
-
-	ldmfd	sp!, {r4-r5}			/* stack is 8 byte aligned */
-	RET
-END(copystr)
-
 #define SAVE_REGS	stmfd	sp!, {r4-r6}
 #define RESTORE_REGS	ldmfd	sp!, {r4-r6}
 
diff --git a/sys/arm64/arm64/copystr.c b/sys/arm64/arm64/copystr.c
diff --git a/sys/conf/files.arm64 b/sys/conf/files.arm64
@@ -134,7 +134,6 @@ arm64/arm64/busdma_machdep.c	standard
 arm64/arm64/bzero.S		standard
 arm64/arm64/clock.c		standard
 arm64/arm64/copyinout.S		standard
-arm64/arm64/copystr.c		standard
 arm64/arm64/cpu_errata.c	standard
 arm64/arm64/cpufunc_asm.S	standard
 arm64/arm64/db_disasm.c		optional	ddb
diff --git a/sys/conf/files.powerpc b/sys/conf/files.powerpc
@@ -241,7 +241,6 @@ powerpc/powerpc/bus_machdep.c	standard
 powerpc/powerpc/busdma_machdep.c standard
 powerpc/powerpc/clock.c		standard
 powerpc/powerpc/copyinout.c	standard
-powerpc/powerpc/copystr.c	standard
 powerpc/powerpc/cpu.c		standard
 powerpc/powerpc/cpu_subr64.S	optional	powerpc64
 powerpc/powerpc/db_disasm.c	optional	ddb
diff --git a/sys/conf/files.riscv b/sys/conf/files.riscv
@@ -37,7 +37,6 @@ riscv/riscv/busdma_bounce.c	standard
 riscv/riscv/busdma_machdep.c	standard
 riscv/riscv/clock.c		standard
 riscv/riscv/copyinout.S		standard
-riscv/riscv/copystr.c		standard
 riscv/riscv/cpufunc_asm.S	standard
 riscv/riscv/db_disasm.c		optional	ddb
 riscv/riscv/db_interface.c	optional	ddb
diff --git a/sys/fs/fuse/fuse_vfsops.c b/sys/fs/fuse/fuse_vfsops.c
@@ -303,8 +303,6 @@ fuse_vfsop_mount(struct mount *mp)
 	int daemon_timeout;
 	int fd;
 
-	size_t len;
-
 	struct cdev *fdev;
 	struct fuse_data *data = NULL;
 	struct thread *td;
@@ -437,8 +435,8 @@ fuse_vfsop_mount(struct mount *mp)
 		strlcat(mp->mnt_stat.f_fstypename, ".", MFSNAMELEN);
 		strlcat(mp->mnt_stat.f_fstypename, subtype, MFSNAMELEN);
 	}
-	copystr(fspec, mp->mnt_stat.f_mntfromname, MNAMELEN - 1, &len);
-	bzero(mp->mnt_stat.f_mntfromname + len, MNAMELEN - len);
+	memset(mp->mnt_stat.f_mntfromname, 0, MNAMELEN);
+	strlcpy(mp->mnt_stat.f_mntfromname, fspec, MNAMELEN);
 	mp->mnt_iosize_max = MAXPHYS;
 
 	/* Now handshaking with daemon */
diff --git a/sys/fs/unionfs/union_vfsops.c b/sys/fs/unionfs/union_vfsops.c
@@ -83,7 +83,6 @@ unionfs_domount(struct mount *mp)
 	char           *tmp;
 	char           *ep;
 	int		len;
-	size_t		done;
 	int		below;
 	uid_t		uid;
 	gid_t		gid;
@@ -304,12 +303,8 @@ unionfs_domount(struct mount *mp)
 	 */
 	vfs_getnewfsid(mp);
 
-	len = MNAMELEN - 1;
-	tmp = mp->mnt_stat.f_mntfromname;
-	copystr((below ? "<below>:" : "<above>:"), tmp, len, &done);
-	len -= done - 1;
-	tmp += done - 1;
-	copystr(target, tmp, len, NULL);
+	snprintf(mp->mnt_stat.f_mntfromname, MNAMELEN, "<%s>:%s",
+	    below ? "below" : "above", target);
 
 	UNIONFSDEBUG("unionfs_mount: from %s, on %s\n",
 	    mp->mnt_stat.f_mntfromname, mp->mnt_stat.f_mntonname);
diff --git a/sys/i386/i386/support.s b/sys/i386/i386/support.s
@@ -233,47 +233,6 @@ ENTRY(memcpy)
 	ret
 END(memcpy)
 
-/*
- * copystr(from, to, maxlen, int *lencopied) - MP SAFE
- */
-ENTRY(copystr)
-	pushl	%esi
-	pushl	%edi
-
-	movl	12(%esp),%esi			/* %esi = from */
-	movl	16(%esp),%edi			/* %edi = to */
-	movl	20(%esp),%edx			/* %edx = maxlen */
-	incl	%edx
-1:
-	decl	%edx
-	jz	4f
-	lodsb
-	stosb
-	orb	%al,%al
-	jnz	1b
-
-	/* Success -- 0 byte reached */
-	decl	%edx
-	xorl	%eax,%eax
-	jmp	6f
-4:
-	/* edx is zero -- return ENAMETOOLONG */
-	movl	$ENAMETOOLONG,%eax
-
-6:
-	/* set *lencopied and return %eax */
-	movl	20(%esp),%ecx
-	subl	%edx,%ecx
-	movl	24(%esp),%edx
-	testl	%edx,%edx
-	jz	7f
-	movl	%ecx,(%edx)
-7:
-	popl	%edi
-	popl	%esi
-	ret
-END(copystr)
-
 ENTRY(bcmp)
 	pushl	%edi
 	pushl	%esi
diff --git a/sys/kern/subr_csan.c b/sys/kern/subr_csan.c
@@ -350,20 +350,12 @@ kcsan_strlen(const char *str)
 	return (s - str);
 }
 
-#undef copystr
 #undef copyin
 #undef copyin_nofault
 #undef copyinstr
 #undef copyout
 #undef copyout_nofault
 
-int
-kcsan_copystr(const void *kfaddr, void *kdaddr, size_t len, size_t *done)
-{
-	kcsan_access((uintptr_t)kdaddr, len, true, false, __RET_ADDR);
-	return copystr(kfaddr, kdaddr, len, done);
-}
-
 int
 kcsan_copyin(const void *uaddr, void *kaddr, size_t len)
 {
diff --git a/sys/mips/mips/support.S b/sys/mips/mips/support.S
@@ -105,12 +105,22 @@
 	.text
 
 /*
- * int copystr(void *kfaddr, void *kdaddr, size_t maxlen, size_t *lencopied)
- * Copy a NIL-terminated string, at most maxlen characters long.  Return the
- * number of characters copied (including the NIL) in *lencopied.  If the
- * string is too long, return ENAMETOOLONG; else return 0.
+ * Copy a null terminated string from the user address space into
+ * the kernel address space.
+ *
+ *	copyinstr(fromaddr, toaddr, maxlength, &lencopied)
+ *		caddr_t fromaddr;
+ *		caddr_t toaddr;
+ *		u_int maxlength;
+ *		u_int *lencopied;
  */
-LEAF(copystr)
+LEAF(copyinstr)
+	PTR_LA		v0, __copyinstr_err
+	blt		a0, zero, __copyinstr_err  # make sure address is in user space
+	GET_CPU_PCPU(v1)
+	PTR_L		v1, PC_CURPCB(v1)
+	PTR_S		v0, U_PCB_ONFAULT(v1)
+
 	move		t0, a2
 	beq		a2, zero, 4f
 1:
@@ -128,37 +138,14 @@ LEAF(copystr)
 	PTR_SUBU	a2, t0, a2		# if the 4th arg was non-NULL
 	PTR_S		a2, 0(a3)
 3:
-	j		ra			# v0 is 0 or ENAMETOOLONG
-	nop
-END(copystr)
 
+	PTR_S		zero, U_PCB_ONFAULT(v1)
+	j		ra
+	nop
 
-/*
- * Copy a null terminated string from the user address space into
- * the kernel address space.
- *
- *	copyinstr(fromaddr, toaddr, maxlength, &lencopied)
- *		caddr_t fromaddr;
- *		caddr_t toaddr;
- *		u_int maxlength;
- *		u_int *lencopied;
- */
-NESTED(copyinstr, CALLFRAME_SIZ, ra)
-	PTR_SUBU	sp, sp, CALLFRAME_SIZ
-	.mask	0x80000000, (CALLFRAME_RA - CALLFRAME_SIZ)
-	PTR_LA	v0, copyerr
-	blt	a0, zero, _C_LABEL(copyerr)  # make sure address is in user space
-	REG_S	ra, CALLFRAME_RA(sp)
-	GET_CPU_PCPU(v1)
-	PTR_L	v1, PC_CURPCB(v1)
-	jal	_C_LABEL(copystr)
-	PTR_S	v0, U_PCB_ONFAULT(v1)
-	REG_L	ra, CALLFRAME_RA(sp)
-	GET_CPU_PCPU(v1)
-	PTR_L	v1, PC_CURPCB(v1)
-	PTR_S	zero, U_PCB_ONFAULT(v1)
-	j	ra
-	PTR_ADDU	sp, sp, CALLFRAME_SIZ
+__copyinstr_err:
+	j		ra
+	li		v0, EFAULT
 END(copyinstr)
 
 /*
diff --git a/sys/powerpc/powerpc/copystr.c b/sys/powerpc/powerpc/copystr.c
diff --git a/sys/riscv/riscv/copystr.c b/sys/riscv/riscv/copystr.c
diff --git a/sys/sys/systm.h b/sys/sys/systm.h
diff --git a/tools/coccinelle/copystr9.cocci b/tools/coccinelle/copystr9.cocci
