Subject: Re: copyinstr() with a zero-length buffer
To: None <tech-kern@netbsd.org>
From: Chuck Silvers <chuq@chuq.com>
List: tech-kern
Date: 02/06/2000 22:44:11
(oops, lost track of this for a while...)


On Tue, Nov 02, 1999 at 12:13:58PM -0500, Charles M. Hannum wrote:
> 
> I observe the following additional problems:
...
> * powerpc doesn't check whether the saved length pointer is non-null.
>   It also uses EACCES rather then EFAULT.


could someone please test this patch for the ppc:

Index: arch/powerpc/powerpc/copyinstr.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/powerpc/powerpc/copyinstr.c,v
retrieving revision 1.2
diff -u -r1.2 copyinstr.c
--- copyinstr.c	1999/01/10 10:24:16	1.2
+++ copyinstr.c	1999/11/07 17:36:17
@@ -44,21 +44,25 @@
 	size_t len;
 	size_t *done;
 {
-	int c;
 	const u_char *up = udaddr;
 	u_char *kp = kaddr;
-	int l;
-	
+	size_t l;
+	int c, rv;
+
+	rv = ENAMETOOLONG;
 	for (l = 0; len-- > 0; l++) {
 		if ((c = fubyte(up++)) < 0) {
-			*done = l;
-			return EACCES;
+			rv = EFAULT;
+			break;
 		}
 		if (!(*kp++ = c)) {
-			*done = l + 1;
-			return 0;
+			l++;
+			rv = 0;
+			break;
 		}
 	}
-	*done = l;
-	return ENAMETOOLONG;
+	if (done != NULL) {
+		*done = l;
+	}
+	return rv;
 }
Index: arch/powerpc/powerpc/copyoutstr.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/powerpc/powerpc/copyoutstr.c,v
retrieving revision 1.2
diff -u -r1.2 copyoutstr.c
--- copyoutstr.c	1999/01/10 10:24:16	1.2
+++ copyoutstr.c	1999/11/07 17:36:17
@@ -46,18 +46,23 @@
 {
 	const u_char *kp = kaddr;
 	u_char *up = udaddr;
-	int l;
-	
+	size_t l;
+	int rv;
+
+	rv = ENAMETOOLONG;
 	for (l = 0; len-- > 0; l++) {
 		if (subyte(up++, *kp) < 0) {
-			*done = l;
-			return EACCES;
+			rv = EFAULT;
+			break;
 		}
 		if (!*kp++) {
-			*done = l + 1;
-			return 0;
+			l++;
+			rv = 0;
+			break;
 		}
 	}
-	*done = l;
-	return ENAMETOOLONG;
+	if (done != NULL) {
+		*done = l;
+	}
+	return rv;
 }
Index: arch/powerpc/powerpc/copystr.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/powerpc/powerpc/copystr.c,v
retrieving revision 1.1
diff -u -r1.1 copystr.c
--- copystr.c	1996/09/30 16:34:43	1.1
+++ copystr.c	1999/11/07 17:36:17
@@ -46,13 +46,18 @@
 	u_char *kfp = kfaddr;
 	u_char *kdp = kdaddr;
 	size_t l;
+	int rv;
 	
+	rv = ENAMETOOLONG;
 	for (l = 0; len-- > 0; l++) {
 		if (!(*kdp++ = *kfp++)) {
-			*done = l + 1;
-			return 0;
+			l++;
+			rv = 0;
+			break;
 		}
 	}
-	*done = l;
-	return ENAMETOOLONG;
+	if (done != NULL) {
+		*done = l;
+	}
+	return rv;
 }








here's the test program I've been using:

#include <string.h>
#include <stdlib.h>
#include <sys/param.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>

#define CHECK_LEN       (NCARGS+2)

static  char    **array = NULL;

int
main(argc, argv, envp)
        int argc;
        char *argv[];
        char *envp[];
{
        int i;

 
        array = (char **)malloc(CHECK_LEN * sizeof(char *));
        for (i = 0; i < (CHECK_LEN - 1); i++)
                array[i] = "/bin/sh";
        array[CHECK_LEN - 1] = NULL;


        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

        array[0] = "z";
        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

        array[0] = (char *)-1L;
        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

        array[0] = (char *)1L;
        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

        array[0] = (char *)0x7f000000;
        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

        array[0] = (char *)0x1f000000;
        errno = 0;
        execve("/bin/sh", array, envp);
        printf("errno %d\n", errno);

	printf("yay!\n");
        exit(0);
}