Subject: Re: copyinstr() with a zero-length buffer
To: None <tech-kern@netbsd.org>
From: Charles M. Hannum <root@ihack.net>
List: tech-kern
Date: 11/02/1999 12:13:58
I observe the following additional problems:
* m68k returns 0 in this case, which is incorrect. [Patch included.]
* mips treats maxlen==0 as maxlen==2^32. [Patch included.]
* ns32k copystr() (but not copy{in,out}str()) returns 0.
* powerpc doesn't check whether the saved length pointer is non-null.
It also uses EACCES rather then EFAULT.
* sh3 is schizo about checking the pointer, and will also look before
the buffer (possibly causing an EFAULT). [Patch included.]
* vax treats maxlen==0 as maxlen==2^32 (at least in the non-locc
case). It also doesn't save the length in EFAULT and ENAMETOOLONG
cases.
(We should perhaps consider whether it even makes sense for the 4th
argument to be null. If not, we could just remove the checks and make
the code slightly faster.)
Index: m68k/m68k/copy.s
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/m68k/m68k/copy.s,v
retrieving revision 1.34
diff -c -2 -r1.34 copy.s
*** copy.s 1999/05/01 19:17:06 1.34
--- copy.s 1999/11/02 17:09:32
***************
*** 249,255 ****
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! clrl d0
movl sp@(12),d1 | count
beq Lcsdone | nothing to copy
subql #1,d1 | predecrement for dbeq
Lcsloop:
--- 249,256 ----
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! moveq #ENAMETOOLONG,d0 | ran out of space
movl sp@(12),d1 | count
beq Lcsdone | nothing to copy
+ clrl d0
subql #1,d1 | predecrement for dbeq
Lcsloop:
***************
*** 282,286 ****
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! clrl d0
movl sp@(12),d1 | count
beq Lcisdone | nothing to copy
--- 283,287 ----
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! moveq #ENAMETOOLONG,d0 | ran out of space
movl sp@(12),d1 | count
beq Lcisdone | nothing to copy
***************
*** 321,325 ****
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! clrl d0
movl sp@(12),d1 | count
beq Lcosdone | nothing to copy
--- 322,326 ----
movl sp@(4),a0 | a0 = fromaddr
movl sp@(8),a1 | a1 = toaddr
! moveq #ENAMETOOLONG,d0 | ran out of space
movl sp@(12),d1 | count
beq Lcosdone | nothing to copy
Index: mips/mips/locore.S
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/mips/mips/locore.S,v
retrieving revision 1.77
diff -c -2 -r1.77 locore.S
*** locore.S 1999/09/25 00:00:38 1.77
--- locore.S 1999/11/02 17:09:41
***************
*** 561,564 ****
--- 561,565 ----
LEAF(copystr)
move t0, a2
+ beq a2, zero, 4f
1:
lbu v0, 0(a0)
***************
*** 569,572 ****
--- 570,574 ----
bne a2, zero, 1b # less than maxlen
addu a1, a1, 1
+ 4:
li v0, ENAMETOOLONG # run out of space
2:
***************
*** 592,595 ****
--- 594,598 ----
sw v0, U_PCB_ONFAULT(v1)
move t0, a2
+ beq a2, zero, 4f
1:
lbu v0, 0(a0)
***************
*** 600,603 ****
--- 603,607 ----
bne a2, zero, 1b
addu a1, a1, 1
+ 4:
li v0, ENAMETOOLONG
2:
***************
*** 623,626 ****
--- 627,631 ----
sw v0, U_PCB_ONFAULT(v1)
move t0, a2
+ beq a2, zero, 4f
1:
lbu v0, 0(a0)
***************
*** 631,634 ****
--- 636,640 ----
bne a2, zero, 1b
addu a1, a1, 1
+ 4:
li v0, ENAMETOOLONG
2:
Index: sh3/sh3/Locore.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/sh3/sh3/Locore.c,v
retrieving revision 1.1
diff -c -2 -r1.1 Locore.c
*** Locore.c 1999/09/13 10:31:26 1.1
--- Locore.c 1999/11/02 17:09:42
***************
*** 252,277 ****
while (cnt--) {
! if ((*to++ = *from++) == 0)
! break;
}
! *lencopied = from - from_top;
!
! if (cnt == 0) {
! if (to >= (char *)VM_MAXUSER_ADDRESS)
! rc = EFAULT;
! else
! rc = ENAMETOOLONG;
! } else
! rc = 0;
curpcb->pcb_onfault = 0;
return rc;
Err999:
! curpcb->pcb_onfault = 0;
! if (lencopied != 0)
*lencopied = from - from_top;
!
return EFAULT;
}
--- 252,276 ----
while (cnt--) {
! if ((*to++ = *from++) == 0) {
! rc = 0;
! goto out;
! }
}
! if (to >= (char *)VM_MAXUSER_ADDRESS)
! rc = EFAULT;
! else
! rc = ENAMETOOLONG;
+ out:
+ if (lencopied)
+ *lencopied = from - from_top;
curpcb->pcb_onfault = 0;
return rc;
Err999:
! if (lencopied)
*lencopied = from - from_top;
! curpcb->pcb_onfault = 0;
return EFAULT;
}
***************
*** 305,332 ****
while (cnt--) {
! if ((*to++ = *from++) == 0)
! break;
}
-
- if (lencopied != NULL)
- *lencopied = from - from_top;
! if (cnt == 0 && *(from - 1) != 0) {
! if (to >= (char *)VM_MAXUSER_ADDRESS)
! rc = EFAULT;
! else
! rc = ENAMETOOLONG;
! } else
! rc = 0;
curpcb->pcb_onfault = 0;
-
return rc;
Err999:
! curpcb->pcb_onfault = 0;
! if (lencopied != 0)
*lencopied = from - from_top;
!
return EFAULT;
}
--- 304,328 ----
while (cnt--) {
! if ((*to++ = *from++) == 0) {
! rc = 0;
! goto out;
! }
}
! if (to >= (char *)VM_MAXUSER_ADDRESS)
! rc = EFAULT;
! else
! rc = ENAMETOOLONG;
+ out:
+ if (lencopied)
+ *lencopied = from - from_top;
curpcb->pcb_onfault = 0;
return rc;
Err999:
! if (lencopied)
*lencopied = from - from_top;
! curpcb->pcb_onfault = 0;
return EFAULT;
}
***************
*** 350,365 ****
for (i = 0; i < maxlen; i++) {
! if ((*to++ = *from++) == NULL)
! break;
}
! if (i == maxlen) {
*lencopied = i;
! return ENAMETOOLONG;
! } else {
! if (lencopied)
! *lencopied = i + 1;
! return 0;
! }
}
--- 346,359 ----
for (i = 0; i < maxlen; i++) {
! if ((*to++ = *from++) == NULL) {
! if (lencopied)
! *lencopied = i + 1;
! return (0);
! }
}
! if (lencopied)
*lencopied = i;
! return (ENAMETOOLONG);
}