The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
Consequently, the file offset may be truncated in the assignment.
Currently, /dev/mem wraps around, which may cause applications to read
or write incorrect regions of memory by accident.
Let's follow POSIX file semantics here and return 0 when reading from
and -EFBIG when writing to an offset that cannot be represented by a
phys_addr_t.
Note that the conditional is optimized out by the compiler if loff_t
has the same size as phys_addr_t.
Signed-off-by: Petr Tesarik <[email protected]>
---
drivers/char/mem.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 92c5937..917403f 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -99,6 +99,9 @@ static ssize_t read_mem(struct file *file, char
__user *buf, ssize_t read, sz;
char *ptr;
+ if (p != *ppos)
+ return 0;
+
if (!valid_phys_addr_range(p, count))
return -EFAULT;
read = 0;
@@ -157,6 +160,9 @@ static ssize_t write_mem(struct file *file, const
char __user *buf, unsigned long copied;
void *ptr;
+ if (p != *ppos)
+ return -EFBIG;
+
if (!valid_phys_addr_range(p, count))
return -EFAULT;
--
1.8.4.5
On Thu, Jan 30, 2014 at 09:48:02AM +0100, Petr Tesarik wrote:
> The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
> Consequently, the file offset may be truncated in the assignment.
> Currently, /dev/mem wraps around, which may cause applications to read
> or write incorrect regions of memory by accident.
Does that really happen? If so, that's a userspace bug, right?
> Let's follow POSIX file semantics here and return 0 when reading from
> and -EFBIG when writing to an offset that cannot be represented by a
> phys_addr_t.
>
> Note that the conditional is optimized out by the compiler if loff_t
> has the same size as phys_addr_t.
>
> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> drivers/char/mem.c | 6 ++++++
> 1 file changed, 6 insertions(+)
What is going to break if we apply this patch? :)
thanks,
greg k-h
On Thu, 30 Jan 2014 05:28:27 -0800
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Jan 30, 2014 at 09:48:02AM +0100, Petr Tesarik wrote:
> > The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
> > Consequently, the file offset may be truncated in the assignment.
> > Currently, /dev/mem wraps around, which may cause applications to read
> > or write incorrect regions of memory by accident.
>
> Does that really happen? If so, that's a userspace bug, right?
In my case, it was a userspace bug, indeed. But debugging would have
been much easier if I saw read() fail with an EOF condition, rather
than pretend that it actually read some bytes (from above 4G) on a
32-bit box.
> > Let's follow POSIX file semantics here and return 0 when reading from
> > and -EFBIG when writing to an offset that cannot be represented by a
> > phys_addr_t.
> >
> > Note that the conditional is optimized out by the compiler if loff_t
> > has the same size as phys_addr_t.
> >
> > Signed-off-by: Petr Tesarik <[email protected]>
> > ---
> > drivers/char/mem.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
>
> What is going to break if we apply this patch? :)
Nothing, unless it was broken already. I mean, if anyone is trying to
play dirty tricks with 32-bit file offset overflow, I'd call such code
sick and broken.
And on 64-bit platforms, the patch does not even change the generated
code.
Petr T
On Thu, Jan 30, 2014 at 03:03:32PM +0100, Petr Tesarik wrote:
> On Thu, 30 Jan 2014 05:28:27 -0800
> Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Thu, Jan 30, 2014 at 09:48:02AM +0100, Petr Tesarik wrote:
> > > The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
> > > Consequently, the file offset may be truncated in the assignment.
> > > Currently, /dev/mem wraps around, which may cause applications to read
> > > or write incorrect regions of memory by accident.
> >
> > Does that really happen? If so, that's a userspace bug, right?
>
> In my case, it was a userspace bug, indeed. But debugging would have
> been much easier if I saw read() fail with an EOF condition, rather
> than pretend that it actually read some bytes (from above 4G) on a
> 32-bit box.
Thats true.
Ok, I'll queue this up after 3.14-rc1 is out, thanks.
greg k-h
On Thu, 30 Jan 2014 07:04:07 -0800
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Jan 30, 2014 at 03:03:32PM +0100, Petr Tesarik wrote:
> > On Thu, 30 Jan 2014 05:28:27 -0800
> > Greg Kroah-Hartman <[email protected]> wrote:
> >
> > > On Thu, Jan 30, 2014 at 09:48:02AM +0100, Petr Tesarik wrote:
> > > > The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
> > > > Consequently, the file offset may be truncated in the assignment.
> > > > Currently, /dev/mem wraps around, which may cause applications to read
> > > > or write incorrect regions of memory by accident.
> > >
> > > Does that really happen? If so, that's a userspace bug, right?
> >
> > In my case, it was a userspace bug, indeed. But debugging would have
> > been much easier if I saw read() fail with an EOF condition, rather
> > than pretend that it actually read some bytes (from above 4G) on a
> > 32-bit box.
>
> Thats true.
>
> Ok, I'll queue this up after 3.14-rc1 is out, thanks.
Hi Greg,
what happened to this patch? I still don't see it in git...
Petr T
On Tue, Apr 01, 2014 at 11:29:30AM +0200, Petr Tesarik wrote:
> On Thu, 30 Jan 2014 07:04:07 -0800
> Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Thu, Jan 30, 2014 at 03:03:32PM +0100, Petr Tesarik wrote:
> > > On Thu, 30 Jan 2014 05:28:27 -0800
> > > Greg Kroah-Hartman <[email protected]> wrote:
> > >
> > > > On Thu, Jan 30, 2014 at 09:48:02AM +0100, Petr Tesarik wrote:
> > > > > The loff_t type may be wider than phys_addr_t (e.g. on 32-bit systems).
> > > > > Consequently, the file offset may be truncated in the assignment.
> > > > > Currently, /dev/mem wraps around, which may cause applications to read
> > > > > or write incorrect regions of memory by accident.
> > > >
> > > > Does that really happen? If so, that's a userspace bug, right?
> > >
> > > In my case, it was a userspace bug, indeed. But debugging would have
> > > been much easier if I saw read() fail with an EOF condition, rather
> > > than pretend that it actually read some bytes (from above 4G) on a
> > > 32-bit box.
> >
> > Thats true.
> >
> > Ok, I'll queue this up after 3.14-rc1 is out, thanks.
>
> Hi Greg,
>
> what happened to this patch? I still don't see it in git...
You got an email when it went into my git tree, and is set to be merged
to Linus for 3.15-rc1.
greg k-h