Re: AUDIT: copy_from_user is a deathtrap.

[Andrew Morton <akpm@zip.com.au>]

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);
}