[PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[Andrei Warkentin]

Currently this returns -EFAULT, but it really should be returning 0,
as in - 0 bytes read or written. This is what you would get by
opening a block device, seeking to the end, and trying to read
something. Additionally, make lseek() check the sought-to offset
to pass the valid_phys_addr_range test.

Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
 drivers/char/mem.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 8fc04b4..02d0b1a 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -98,7 +98,7 @@ static ssize_t read_mem(struct file *file, char __user *buf,
 	char *ptr;
 
 	if (!valid_phys_addr_range(p, count))
-		return -EFAULT;
+		return 0;
 	read = 0;
 #ifdef __ARCH_HAS_NO_PAGE_ZERO_MAPPED
 	/* we don't have page 0 mapped on sparc and m68k.. */
@@ -156,7 +156,7 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
 	void *ptr;
 
 	if (!valid_phys_addr_range(p, count))
-		return -EFAULT;
+		return 0;
 
 	written = 0;
 
@@ -710,6 +710,11 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 	case SEEK_CUR:
 		offset += file->f_pos;
 	case SEEK_SET:
+		if (!valid_phys_addr_range(offset, 0)) {
+			ret = -EINVAL;
+			break;
+		}
+
 		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
 		if ((unsigned long long)offset >= ~0xFFFULL) {
 			ret = -EOVERFLOW;
-- 
1.7.4.1

Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[Andrei Warkentin]

Hi Peter,

----- Original Message -----
> From: "Andrei Warkentin" <andreiw@vmware.com>
> To: linux-kernel@vger.kernel.org
> Cc: "Andrei Warkentin" <andreiw@vmware.com>, "H. Peter Anvin" <hpa@zytor.com>
> Sent: Wednesday, November 9, 2011 6:31:19 PM
> Subject: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.
> 
> Currently this returns -EFAULT, but it really should be returning 0,
> as in - 0 bytes read or written. This is what you would get by
> opening a block device, seeking to the end, and trying to read
> something. Additionally, make lseek() check the sought-to offset
> to pass the valid_phys_addr_range test.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---

I know you're very busy, but I was wondering if you could
give your two cents on this change. This change grew from
our discussion on LKML last week.

Thanks ahead,
A

Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[H. Peter Anvin]

On 11/14/2011 01:21 PM, Andrei Warkentin wrote:
>
> I know you're very busy, but I was wondering if you could
> give your two cents on this change. This change grew from
> our discussion on LKML last week.
>
> Thanks ahead,
> A

EOF seems wrong to me (not as bad as EFAULT, but still wrong).

	-hpa

Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[Andrei Warkentin]

Hi Peter,

----- Original Message -----
> From: "H. Peter Anvin" <hpa@zytor.com>
> To: "Andrei Warkentin" <awarkentin@vmware.com>
> Cc: linux-kernel@vger.kernel.org
> Sent: Monday, November 14, 2011 4:59:58 PM
> Subject: Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.
> 
> On 11/14/2011 01:21 PM, Andrei Warkentin wrote:
> >
> > I know you're very busy, but I was wondering if you could
> > give your two cents on this change. This change grew from
> > our discussion on LKML last week.
> >
> > Thanks ahead,
> > A
> 
> EOF seems wrong to me (not as bad as EFAULT, but still wrong).

Although that was what we had discussed, I do not return -EOF in the patch. As you
mention - that would be wrong. I played around with other devices (block, actually),
and if you attempt to read(2) beyond the end, read(2) simply returns 0, as in
zero bytes read out. Of course, lseek(2) beyond the end should return -EINVAL, as
well, so that is what that patch I CCd you on accomplishes.

A

Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[H. Peter Anvin]

On 11/14/2011 02:11 PM, Andrei Warkentin wrote:
>>
>> EOF seems wrong to me (not as bad as EFAULT, but still wrong).
>
> Although that was what we had discussed, I do not return -EOF in the patch. As you
> mention - that would be wrong. I played around with other devices (block, actually),
> and if you attempt to read(2) beyond the end, read(2) simply returns 0, as in
> zero bytes read out. Of course, lseek(2) beyond the end should return -EINVAL, as
> well, so that is what that patch I CCd you on accomplishes.
>

0 is EOF.

This is not a block device -- comparing to block devices is pointless. 
There is no "end" to /dev/mem, so this is a totally meaningless comparison.

	-hpa

Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "H. Peter Anvin" <hpa@zytor.com>
> To: "Andrei Warkentin" <awarkentin@vmware.com>
> Cc: linux-kernel@vger.kernel.org
> Sent: Monday, November 14, 2011 5:12:35 PM
> Subject: Re: [PATCH] /dev/mem: Fix wrong error on accessing beyond valid memory addresses.
> 
> On 11/14/2011 02:11 PM, Andrei Warkentin wrote:
> >>
> >> EOF seems wrong to me (not as bad as EFAULT, but still wrong).
> >
> > Although that was what we had discussed, I do not return -EOF in
> > the patch. As you
> > mention - that would be wrong. I played around with other devices
> > (block, actually),
> > and if you attempt to read(2) beyond the end, read(2) simply
> > returns 0, as in
> > zero bytes read out. Of course, lseek(2) beyond the end should
> > return -EINVAL, as
> > well, so that is what that patch I CCd you on accomplishes.
> >
> 
> 0 is EOF.
> 

Ah, whoops, silly me.

> This is not a block device -- comparing to block devices is
> pointless.
> There is no "end" to /dev/mem, so this is a totally meaningless
> comparison.

Oh ok, I looked at other arches, like IA64, and now see why
using valid_phys_addr_range as an "end" test is meaningless.

Thanks,
A