Messages in this thread | | | Subject | Re: [PATCH] Revert "drivers: char: mem: Check {read,write}_kmem() addresses" | From | Robin Murphy <> | Date | Wed, 4 Jan 2017 11:32:42 +0000 |
| |
On 04/01/17 09:22, Greg KH wrote: > On Tue, Jan 03, 2017 at 09:50:52PM +0100, Jason A. Donenfeld wrote: >> This reverts commit 148a1bc84398039e2b96ff78678c4d9a67f81452. >> >> This commit was intended to fix a problem, but it did not do so >> correctly, resulting in /dev/kmem being entirely broken. So, revert this >> commit until a different solution is found. The following PoC shows >> breakage bisecting to this commit. On working systems, it returns 0 and >> prints the last message. On broken systems containing this commit, it >> exits returning EIO. >> >> #define _FILE_OFFSET_BITS 64 >> #include <unistd.h> >> #include <fcntl.h> >> #include <stdio.h> >> #include <string.h> >> #include <stdlib.h> >> #include <errno.h> >> #include <sys/types.h> >> #include <sys/socket.h> >> #include <netinet/in.h> >> >> int main(int argc, char *argv[]) >> { >> int fd, sock; >> FILE *f; >> char line[256], *ptr; >> unsigned long addr = 0; >> struct sockaddr_in saddr = { .sin_family = AF_INET, .sin_port = 55193 }; >> >> sock = socket(AF_INET, SOCK_DGRAM, 0); >> if (sock < 0) { >> perror("socket"); >> return errno; >> } >> if (bind(sock, (struct sockaddr *)&saddr, sizeof(saddr)) < 0) { >> perror("bind"); >> return errno; >> } >> f = fopen("/proc/net/udp", "r"); >> if (!f) { >> perror("fopen(/proc/net/udp)"); >> return errno; >> } >> if (!fgets(line, 256, f) || !fgets(line, 256, f)) { >> perror("fgets"); >> return errno; >> } >> fclose(f); >> ptr = line + strlen(line) - 1; >> while (*--ptr == ' '); >> while (*--ptr != ' '); >> while (*(--ptr - 1) != ' '); >> addr = strtoul(ptr, NULL, 16); >> >> printf("Attempting to read from skbuff at 0x%lx\n", addr); >> >> fd = open("/dev/kmem", O_RDONLY); >> if (fd < 0) { >> perror("open(/dev/kmem)"); >> return errno; >> } >> if (lseek(fd, addr, SEEK_SET) == (off_t)-1) { >> perror("lseek"); >> return errno; >> } >> if (read(fd, &addr, sizeof(addr)) != sizeof(addr)) { >> perror("read"); >> return errno; >> } >> >> printf("It worked, reading the value %lx!\n", addr); >> close(fd); >> close(sock); >> return 0; >> } >> >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> >> Cc: Robin Murphy <robin.murphy@arm.com> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> drivers/char/mem.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/char/mem.c b/drivers/char/mem.c >> index 5bb1985ec484..a33163dbb913 100644 >> --- a/drivers/char/mem.c >> +++ b/drivers/char/mem.c >> @@ -381,9 +381,6 @@ static ssize_t read_kmem(struct file *file, char __user *buf, >> char *kbuf; /* k-addr because vread() takes vmlist_lock rwlock */ >> int err = 0; >> >> - if (!pfn_valid(PFN_DOWN(p))) >> - return -EIO; >> - >> read = 0; >> if (p < (unsigned long) high_memory) { >> low_count = count; >> @@ -512,9 +509,6 @@ static ssize_t write_kmem(struct file *file, const char __user *buf, >> char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */ >> int err = 0; >> >> - if (!pfn_valid(PFN_DOWN(p))) >> - return -EIO; >> - >> if (p < (unsigned long) high_memory) { >> unsigned long to_write = min_t(unsigned long, count, >> (unsigned long)high_memory - p); > > Ick, I hate /dev/kmem... > > Robin and Kefeng, any comments? Reverting seems correct at this point > in time, any objection?
Having spotted the (embarassingly obvious) problem, the real fix is an even simpler two-liner. Patch incoming...
Robin.
> thanks, > > greg k-h >
| |