[PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Robin Murphy]

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

Re: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Russell King - ARM Linux]

On Tue, May 31, 2016 at 01:52:45PM +0100, Robin Murphy wrote:
> 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.

Returning -EFAULT because the kernel-side address (iow, file offset) is
invalid is not particularly nice:

NAME
       read - read from a file descriptor

ERRORS
       EFAULT buf is outside your accessible address space.

Latest POSIX has:

ENXIO
	A request was made of a nonexistent device, or the
	request was outside the capabilities of the device.

which to me looks like a better error code to return, as file offsets
which are not valid can be interpreted as being "outside the
capabilities of the device".  EFAULT has always on Linux meant that
the user passed an invalid userspace buffer.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Robin Murphy]

On 31/05/16 14:08, Russell King - ARM Linux wrote:
> On Tue, May 31, 2016 at 01:52:45PM +0100, Robin Murphy wrote:
>> 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.
>
> Returning -EFAULT because the kernel-side address (iow, file offset) is
> invalid is not particularly nice:
>
> NAME
>         read - read from a file descriptor
>
> ERRORS
>         EFAULT buf is outside your accessible address space.
>
> Latest POSIX has:
>
> ENXIO
> 	A request was made of a nonexistent device, or the
> 	request was outside the capabilities of the device.
>
> which to me looks like a better error code to return, as file offsets
> which are not valid can be interpreted as being "outside the
> capabilities of the device".  EFAULT has always on Linux meant that
> the user passed an invalid userspace buffer.

Good point - seems I failed to twig that the error code in the x86 case 
is still effectively falling out of the "fault with the user address" 
path. ENXIO indeed sounds more reasonable, thanks.

Robin.

Re: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Catalin Marinas]

On Tue, May 31, 2016 at 01:52:45PM +0100, Robin Murphy wrote:
> 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;

Since the /dev/kmem interface is about kernel virtual address rather
than physical (like /dev/mem), the pfn may not always be mapped. I think
a better check would be to use kern_addr_valid(kaddr) just before
copy_(to|from)_user (a similar approach is taken by read_kcore()). The
downside is that it breaks a couple of configurations where
kern_addr_valid() is 0:

- x86_32 with !CONFIG_FLATMEM
- alpha with CONFIG_DISCONTIGMEM

-- 
Catalin

Re: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Robin Murphy]

On 31/05/16 14:46, Catalin Marinas wrote:
> On Tue, May 31, 2016 at 01:52:45PM +0100, Robin Murphy wrote:
>> 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;
>
> Since the /dev/kmem interface is about kernel virtual address rather
> than physical (like /dev/mem), the pfn may not always be mapped. I think
> a better check would be to use kern_addr_valid(kaddr) just before
> copy_(to|from)_user (a similar approach is taken by read_kcore()). The
> downside is that it breaks a couple of configurations where
> kern_addr_valid() is 0:

Well, the mmap() case, which is arguably the "normal" access method, 
looks to have been enforcing pfn_valid since pretty much forever[1] so I 
struggle to imagine how much anyone will actually care. In my view it's 
more just that "do a silly thing and get an error" seems preferable to 
"do a silly thing and get a scary backtrace".

Robin.

[1]:http://lwn.net/Articles/147901/ - I particularly enjoyed 
"[...]chances are that /dev/kmem will not survive into 2.6.14"

>
> - x86_32 with !CONFIG_FLATMEM
> - alpha with CONFIG_DISCONTIGMEM
>

Re: [PATCH] drivers: char: mem: Check {read,write}_kmem() addresses

[Kefeng Wang]

On 2016/5/31 21:40, Robin Murphy wrote:
> On 31/05/16 14:08, Russell King - ARM Linux wrote:
>> On Tue, May 31, 2016 at 01:52:45PM +0100, Robin Murphy wrote:
>>> 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.
>>
>> Returning -EFAULT because the kernel-side address (iow, file offset) is
>> invalid is not particularly nice:
>>
>> NAME
>>         read - read from a file descriptor
>>
>> ERRORS
>>         EFAULT buf is outside your accessible address space.
>>
>> Latest POSIX has:
>>
>> ENXIO
>>     A request was made of a nonexistent device, or the
>>     request was outside the capabilities of the device.
>>
>> which to me looks like a better error code to return, as file offsets
>> which are not valid can be interpreted as being "outside the
>> capabilities of the device".  EFAULT has always on Linux meant that
>> the user passed an invalid userspace buffer.
> 
> Good point - seems I failed to twig that the error code in the x86 case is still effectively falling out of the "fault with the user address" path. ENXIO indeed sounds more reasonable, thanks.

Hi Robin, thanks for you fix, return -EIO like mmap_kmem() is better to me.

BRs,
Kefeng


> 
> Robin.
> 
> .
> 

[PATCH v2] drivers: char: mem: Check {read,write}_kmem() addresses

[Robin Murphy]

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-side 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 user-side fixup handler and the chain of events which
transpires ends up returning an error as one might expect, even if it's
an inappropriate -EFAULT. On other architectures, though, the read is
not covered by the fixup entry for the write, and we get a big scary
"Unable to hande kernel paging request..." dump.

The more typical use-case of mmap_kmem() has always (within living
memory at least) returned -EIO for addresses which don't satisfy
pfn_valid(), 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>
---

v2: Make the error code consistent and more appropriate, tweak commit message.

 drivers/char/mem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 71025c2f6bbb..e1b8d960341f 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 -EIO;
+
 	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 -EIO;
+
 	if (p < (unsigned long) high_memory) {
 		unsigned long to_write = min_t(unsigned long, count,
 					       (unsigned long)high_memory - p);
-- 
2.8.1.dirty