From: Andrew Morton <akpm@zip.com.au> To: Alan Cox <alan@lxorguk.ukuu.org.uk> Subject: Re: AUDIT: copy_from_user is a deathtrap. Date: Tue, 21 May 2002 14:46:08 -0700 Cc: Pavel Machek <pavel@suse.cz>, Linus Torvalds <torvalds@transmeta.com>, Rusty Russell <rusty@rustcorp.com.au>, linux-kernel@vger.kernel.org Alan Cox wrote: > > > So if you pass bad pointer to read(), why would you expect "number of > > bytes read" return? Its true that kernel can't simply not return > > Because the standard says either you return the errorcode and no data > is transferred or for a partial I/O you return how much was done. Its > not neccessarily about accuracy either. If you do a 4k copy_from_user and > error after for some reason returning -Esomething thats fine providing you > didnt do anything that consumed data or shifted the file position etc Is it safe to stick a nose in here with some irrelevancies? Pavel makes a reasonable point that copy_*_user may elect to copy the data in something other than strictly ascending user virtual addresses. In which case it's not possible to return a sane "how much was copied" number. And copy_*_user is buggy at present: it doesn't correctly handle the case where the source and destination of the copy are overlapping in the same physical page. Example code below. One fix is to do the copy with descending addresses if src<dest or whatever. But then how to return the number of bytes?? Also, I see all these noises from x86 gurus about how copy_*_user() could be sped up heaps with fancy CPU-specific features. Could someone actually alight from butt and code that up? akpm-1:/usr/src/ext3/tools> ./copy-user-test foo aabcddfghhjkllnopprsttvwxx #include <stdio.h> #include <unistd.h> #include <fcntl.h> #include <stdlib.h> #include <errno.h> #include <string.h> #include <sys/mman.h> int main(int argc, char *argv[]) { int fd; char *filename; char *mapped_mem; char buf[26]; int i; if (argc != 2) { fprintf(stderr, "Usage; %s filename\n", argv[0]); exit(1); } filename = argv[1]; fd = open(filename, O_RDWR|O_TRUNC|O_CREAT, 0666); if (fd < 0) { fprintf(stderr, "%s: Cannot open `%s': %s\n", argv[0], filename, strerror(errno)); exit(1); } for (i = 0; i < 26; i++) buf[i] = 'a' + i; mapped_mem = mmap(0, 26, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); if (mapped_mem == 0) { perror("mmap"); exit(1); } write(fd, buf, 26); lseek(fd, 1, SEEK_SET); write(fd, mapped_mem, 25); msync(mapped_mem, 26, MS_SYNC); munmap(mapped_mem, 26); close(fd); { char *p = malloc(strlen(filename) + 20); sprintf(p, "cat %s ; echo", filename); system(p); } exit(0); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/