* [PATCH] drivers: char: mem: Fix thinko in kmem address checks @ 2017-01-04 11:37 Robin Murphy 2017-01-04 11:45 ` Greg KH ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Robin Murphy @ 2017-01-04 11:37 UTC (permalink / raw) To: gregkh, Jason; +Cc: linux-kernel, stable When borrowing the pfn_valid() check from mmap_kmem(), somebody managed to get physical and virtual addresses spectacularly muddled up, such that we've ended up with checks for one being the other. Whilst this does indeed prevent out-of-bounds accesses crashing, on most systems it also prevents the more desirable use-case of working at all ever. Check the *virtual* offset correctly for what it is. Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> CC: stable@vger.kernel.org Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses") Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/char/mem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 5bb1985ec484..bdc6a4018604 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -381,7 +381,7 @@ 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))) + if (!virt_addr_valid(p)) return -EIO; read = 0; @@ -512,7 +512,7 @@ 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))) + if (!virt_addr_valid(p)) return -EIO; if (p < (unsigned long) high_memory) { -- 2.10.2.dirty ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: char: mem: Fix thinko in kmem address checks 2017-01-04 11:37 [PATCH] drivers: char: mem: Fix thinko in kmem address checks Robin Murphy @ 2017-01-04 11:45 ` Greg KH 2017-01-04 17:19 ` Greg KH 2017-01-04 16:46 ` kbuild test robot 2017-01-05 17:15 ` [PATCH v2] drivers: char: mem: Fix thinkos " Robin Murphy 2 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2017-01-04 11:45 UTC (permalink / raw) To: Robin Murphy; +Cc: Jason, linux-kernel, stable On Wed, Jan 04, 2017 at 11:37:49AM +0000, Robin Murphy wrote: > When borrowing the pfn_valid() check from mmap_kmem(), somebody managed "sombody"? :) > to get physical and virtual addresses spectacularly muddled up, such > that we've ended up with checks for one being the other. Whilst this > does indeed prevent out-of-bounds accesses crashing, on most systems it > also prevents the more desirable use-case of working at all ever. > > Check the *virtual* offset correctly for what it is. > > Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> > CC: stable@vger.kernel.org > Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses") > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/char/mem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > index 5bb1985ec484..bdc6a4018604 100644 > --- a/drivers/char/mem.c > +++ b/drivers/char/mem.c > @@ -381,7 +381,7 @@ 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))) > + if (!virt_addr_valid(p)) > return -EIO; > > read = 0; > @@ -512,7 +512,7 @@ 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))) > + if (!virt_addr_valid(p)) > return -EIO; > > if (p < (unsigned long) high_memory) { Jason, can you verify this fixes your test case? thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: char: mem: Fix thinko in kmem address checks 2017-01-04 11:45 ` Greg KH @ 2017-01-04 17:19 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2017-01-04 17:19 UTC (permalink / raw) To: Robin Murphy; +Cc: Jason, linux-kernel, stable On Wed, Jan 04, 2017 at 12:45:32PM +0100, Greg KH wrote: > On Wed, Jan 04, 2017 at 11:37:49AM +0000, Robin Murphy wrote: > > When borrowing the pfn_valid() check from mmap_kmem(), somebody managed > > "sombody"? :) > > > to get physical and virtual addresses spectacularly muddled up, such > > that we've ended up with checks for one being the other. Whilst this > > does indeed prevent out-of-bounds accesses crashing, on most systems it > > also prevents the more desirable use-case of working at all ever. > > > > Check the *virtual* offset correctly for what it is. > > > > Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> > > CC: stable@vger.kernel.org > > Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses") > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > --- > > drivers/char/mem.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > > index 5bb1985ec484..bdc6a4018604 100644 > > --- a/drivers/char/mem.c > > +++ b/drivers/char/mem.c > > @@ -381,7 +381,7 @@ 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))) > > + if (!virt_addr_valid(p)) > > return -EIO; > > > > read = 0; > > @@ -512,7 +512,7 @@ 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))) > > + if (!virt_addr_valid(p)) > > return -EIO; > > > > if (p < (unsigned long) high_memory) { > > Jason, can you verify this fixes your test case? Well, it fails kbuild testing, so can you try it again? thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: char: mem: Fix thinko in kmem address checks 2017-01-04 11:37 [PATCH] drivers: char: mem: Fix thinko in kmem address checks Robin Murphy 2017-01-04 11:45 ` Greg KH @ 2017-01-04 16:46 ` kbuild test robot 2017-01-05 17:15 ` [PATCH v2] drivers: char: mem: Fix thinkos " Robin Murphy 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2017-01-04 16:46 UTC (permalink / raw) To: Robin Murphy; +Cc: kbuild-all, gregkh, Jason, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 2299 bytes --] Hi Robin, [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on v4.10-rc2 next-20170104] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Robin-Murphy/drivers-char-mem-Fix-thinko-in-kmem-address-checks/20170104-235754 config: tile-allmodconfig (attached as .config) compiler: tilegx-linux-gcc (GCC) 4.6.2 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=tile All warnings (new ones prefixed by >>): drivers/char/mem.c: In function 'read_kmem': >> drivers/char/mem.c:384:2: warning: passing argument 1 of 'kaddr_to_pfn' makes pointer from integer without a cast [enabled by default] arch/tile/include/asm/page.h:281:29: note: expected 'const volatile void *' but argument is of type 'long unsigned int' drivers/char/mem.c: In function 'write_kmem': drivers/char/mem.c:515:2: warning: passing argument 1 of 'kaddr_to_pfn' makes pointer from integer without a cast [enabled by default] arch/tile/include/asm/page.h:281:29: note: expected 'const volatile void *' but argument is of type 'long unsigned int' vim +/kaddr_to_pfn +384 drivers/char/mem.c 368 369 vma->vm_pgoff = pfn; 370 return mmap_mem(file, vma); 371 } 372 373 /* 374 * This function reads the *virtual* memory as seen by the kernel. 375 */ 376 static ssize_t read_kmem(struct file *file, char __user *buf, 377 size_t count, loff_t *ppos) 378 { 379 unsigned long p = *ppos; 380 ssize_t low_count, read, sz; 381 char *kbuf; /* k-addr because vread() takes vmlist_lock rwlock */ 382 int err = 0; 383 > 384 if (!virt_addr_valid(p)) 385 return -EIO; 386 387 read = 0; 388 if (p < (unsigned long) high_memory) { 389 low_count = count; 390 if (count > (unsigned long)high_memory - p) 391 low_count = (unsigned long)high_memory - p; 392 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 47087 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] drivers: char: mem: Fix thinkos in kmem address checks 2017-01-04 11:37 [PATCH] drivers: char: mem: Fix thinko in kmem address checks Robin Murphy 2017-01-04 11:45 ` Greg KH 2017-01-04 16:46 ` kbuild test robot @ 2017-01-05 17:15 ` Robin Murphy 2017-01-10 17:49 ` Greg KH 2 siblings, 1 reply; 7+ messages in thread From: Robin Murphy @ 2017-01-05 17:15 UTC (permalink / raw) To: gregkh, Jason; +Cc: linux-kernel, stable When borrowing the pfn_valid() check from mmap_kmem(), somebody managed to get physical and virtual addresses spectacularly muddled up, such that we've ended up with checks for one being the other. Whilst this does indeed prevent out-of-bounds accesses crashing, on most systems it also prevents the more desirable use-case of working at all ever. Check the *virtual* offset correctly for what it is. Furthermore, do so in the right place - a read or write may span multiple pages, so a single up-front check is insufficient. High memory accesses already have a similar validity check just before the copy_to_user() call, so just make the low memory path fully consistent with that. Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> CC: stable@vger.kernel.org Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses") Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- Third time lucky... And if there's some other problem with this one then I guess we may as well just go ahead with Jason's revert, forget the whole thing, and let 'cat /dev/kmem' go back to crashing on non-x86 :) Robin. drivers/char/mem.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 5bb1985ec484..6d9cc2d39d22 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; @@ -412,6 +409,8 @@ static ssize_t read_kmem(struct file *file, char __user *buf, * by the kernel or data corruption may occur */ kbuf = xlate_dev_kmem_ptr((void *)p); + if (!virt_addr_valid(kbuf)) + return -ENXIO; if (copy_to_user(buf, kbuf, sz)) return -EFAULT; @@ -482,6 +481,8 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf, * corruption may occur. */ ptr = xlate_dev_kmem_ptr((void *)p); + if (!virt_addr_valid(ptr)) + return -ENXIO; copied = copy_from_user(ptr, buf, sz); if (copied) { @@ -512,9 +513,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); -- 2.10.2.dirty ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers: char: mem: Fix thinkos in kmem address checks 2017-01-05 17:15 ` [PATCH v2] drivers: char: mem: Fix thinkos " Robin Murphy @ 2017-01-10 17:49 ` Greg KH 2017-01-11 19:18 ` Jason A. Donenfeld 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2017-01-10 17:49 UTC (permalink / raw) To: Robin Murphy; +Cc: Jason, linux-kernel, stable On Thu, Jan 05, 2017 at 05:15:01PM +0000, Robin Murphy wrote: > When borrowing the pfn_valid() check from mmap_kmem(), somebody managed > to get physical and virtual addresses spectacularly muddled up, such > that we've ended up with checks for one being the other. Whilst this > does indeed prevent out-of-bounds accesses crashing, on most systems > it also prevents the more desirable use-case of working at all ever. > > Check the *virtual* offset correctly for what it is. Furthermore, do > so in the right place - a read or write may span multiple pages, so a > single up-front check is insufficient. High memory accesses already > have a similar validity check just before the copy_to_user() call, so > just make the low memory path fully consistent with that. > > Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> > CC: stable@vger.kernel.org > Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses") > Signed-off-by: Robin Murphy <robin.murphy@arm.com> Jason, did this patch fix your issue? thanks, greg k-h > --- > > Third time lucky... And if there's some other problem with this one then > I guess we may as well just go ahead with Jason's revert, forget the whole > thing, and let 'cat /dev/kmem' go back to crashing on non-x86 :) > > Robin. > > drivers/char/mem.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > index 5bb1985ec484..6d9cc2d39d22 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; > @@ -412,6 +409,8 @@ static ssize_t read_kmem(struct file *file, char __user *buf, > * by the kernel or data corruption may occur > */ > kbuf = xlate_dev_kmem_ptr((void *)p); > + if (!virt_addr_valid(kbuf)) > + return -ENXIO; > > if (copy_to_user(buf, kbuf, sz)) > return -EFAULT; > @@ -482,6 +481,8 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf, > * corruption may occur. > */ > ptr = xlate_dev_kmem_ptr((void *)p); > + if (!virt_addr_valid(ptr)) > + return -ENXIO; > > copied = copy_from_user(ptr, buf, sz); > if (copied) { > @@ -512,9 +513,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); > -- > 2.10.2.dirty ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers: char: mem: Fix thinkos in kmem address checks 2017-01-10 17:49 ` Greg KH @ 2017-01-11 19:18 ` Jason A. Donenfeld 0 siblings, 0 replies; 7+ messages in thread From: Jason A. Donenfeld @ 2017-01-11 19:18 UTC (permalink / raw) To: Greg KH; +Cc: Robin Murphy, LKML, stable Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> My extract-keys [1] utility works again: $ sudo ./extract-keys wg0 0x08d3c7a3 IbbC5O9g2dTZBeLQoivmKZtdSGqufU+M2s2oaFzMfYA= 0xdf917af6 3cUm2mXNaZVzWDmJIZ9XlPpvJ63GH1uyUwKLN1g1Dhs= [1] https://git.zx2c4.com/WireGuard/tree/contrib/examples/extract-keys ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-11 19:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-04 11:37 [PATCH] drivers: char: mem: Fix thinko in kmem address checks Robin Murphy 2017-01-04 11:45 ` Greg KH 2017-01-04 17:19 ` Greg KH 2017-01-04 16:46 ` kbuild test robot 2017-01-05 17:15 ` [PATCH v2] drivers: char: mem: Fix thinkos " Robin Murphy 2017-01-10 17:49 ` Greg KH 2017-01-11 19:18 ` Jason A. Donenfeld
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).