From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966925AbdADJVw (ORCPT ); Wed, 4 Jan 2017 04:21:52 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:58764 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966900AbdADJVu (ORCPT ); Wed, 4 Jan 2017 04:21:50 -0500 Date: Wed, 4 Jan 2017 10:22:11 +0100 From: Greg KH To: "Jason A. Donenfeld" Cc: LKML , Robin Murphy , Kefeng Wang Subject: Re: [PATCH] Revert "drivers: char: mem: Check {read,write}_kmem() addresses" Message-ID: <20170104092211.GA31677@kroah.com> References: <20170103205052.30517-1-Jason@zx2c4.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170103205052.30517-1-Jason@zx2c4.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > #include > #include > #include > #include > #include > #include > #include > #include > > 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 > Cc: Robin Murphy > Cc: Kefeng Wang > Cc: Greg Kroah-Hartman > --- > 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? thanks, greg k-h