From: Robin Murphy <robin.murphy@arm.com> To: arnd@arndb.de, gregkh@linuxfoundation.org Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, wangkefeng.wang@huawei.com Subject: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses Date: Tue, 31 May 2016 13:52:45 +0100 [thread overview] Message-ID: <5e293c103f0a86f512d71f17091a55e9746e711f.1464698496.git.robin.murphy@arm.com> (raw) Arriving at read_kmem() with an offset representing a bogus kernel address (e.g. 0 from a simple "cat /dev/kmem") leads to copy_to_user faulting on the kernel-space read. x86_64 happens to get away with this since the optimised implementation uses "rep movs*", thus the user write (which is allowed to fault) and the kernel read are the same instruction, the kernel-side fault falls into the userspace fixup handler and a chain of events transpires leading to returning the expected -EFAULT. On other architectures, though, the read is not covered by the fixup entry for the write, and we get a straightforward "Unable to hande kernel paging request..." dump. The more typical use-case of mmap_kmem() already validates the address with pfn_valid() as one might expect, so let's make that consistent across {read,write}_kem() too. Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- I'm not sure if this warrants going to stable or not, as it's really just making an existing failure case more graceful and less confusing. drivers/char/mem.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 71025c2f6bbb..64c766023b15 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -384,6 +384,9 @@ 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 -EFAULT; + read = 0; if (p < (unsigned long) high_memory) { low_count = count; @@ -512,6 +515,9 @@ 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 -EFAULT; + if (p < (unsigned long) high_memory) { unsigned long to_write = min_t(unsigned long, count, (unsigned long)high_memory - p); -- 2.8.1.dirty
WARNING: multiple messages have this Message-ID (diff)
From: robin.murphy@arm.com (Robin Murphy) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses Date: Tue, 31 May 2016 13:52:45 +0100 [thread overview] Message-ID: <5e293c103f0a86f512d71f17091a55e9746e711f.1464698496.git.robin.murphy@arm.com> (raw) Arriving at read_kmem() with an offset representing a bogus kernel address (e.g. 0 from a simple "cat /dev/kmem") leads to copy_to_user faulting on the kernel-space read. x86_64 happens to get away with this since the optimised implementation uses "rep movs*", thus the user write (which is allowed to fault) and the kernel read are the same instruction, the kernel-side fault falls into the userspace fixup handler and a chain of events transpires leading to returning the expected -EFAULT. On other architectures, though, the read is not covered by the fixup entry for the write, and we get a straightforward "Unable to hande kernel paging request..." dump. The more typical use-case of mmap_kmem() already validates the address with pfn_valid() as one might expect, so let's make that consistent across {read,write}_kem() too. Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- I'm not sure if this warrants going to stable or not, as it's really just making an existing failure case more graceful and less confusing. drivers/char/mem.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 71025c2f6bbb..64c766023b15 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -384,6 +384,9 @@ 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 -EFAULT; + read = 0; if (p < (unsigned long) high_memory) { low_count = count; @@ -512,6 +515,9 @@ 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 -EFAULT; + if (p < (unsigned long) high_memory) { unsigned long to_write = min_t(unsigned long, count, (unsigned long)high_memory - p); -- 2.8.1.dirty
next reply other threads:[~2016-05-31 12:52 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-05-31 12:52 Robin Murphy [this message] 2016-05-31 12:52 ` [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses Robin Murphy 2016-05-31 13:08 ` Russell King - ARM Linux 2016-05-31 13:08 ` Russell King - ARM Linux 2016-05-31 13:40 ` Robin Murphy 2016-05-31 13:40 ` Robin Murphy 2016-06-01 6:42 ` Kefeng Wang 2016-06-01 6:42 ` Kefeng Wang 2016-05-31 13:46 ` Catalin Marinas 2016-05-31 13:46 ` Catalin Marinas 2016-05-31 16:45 ` Robin Murphy 2016-05-31 16:45 ` Robin Murphy 2016-06-01 18:21 ` [PATCH v2] " Robin Murphy 2016-06-01 18:21 ` Robin Murphy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5e293c103f0a86f512d71f17091a55e9746e711f.1464698496.git.robin.murphy@arm.com \ --to=robin.murphy@arm.com \ --cc=arnd@arndb.de \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=wangkefeng.wang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.