Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-18 Thread Boris Brezillon
On Sat,  7 Jul 2018 05:37:22 +0200
Jann Horn  wrote:

> The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> pread/pwrite syscalls bypass this.
> 
> I haven't found any codepath on which this actually causes dangerous
> behavior, but it seems like a sensible change anyway.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn 

Applied.

Thanks,

Boris

> ---
>  drivers/mtd/mtdchar.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index cd67c85cc87d..02389528f622 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> __user *buf, size_t count,
>  
>   pr_debug("MTD_read\n");
>  
> - if (*ppos + count > mtd->size)
> - count = mtd->size - *ppos;
> + if (*ppos + count > mtd->size) {
> + if (*ppos < mtd->size)
> + count = mtd->size - *ppos;
> + else
> + count = 0;
> + }
>  
>   if (!count)
>   return 0;
> @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> char __user *buf, size_t c
>  
>   pr_debug("MTD_write\n");
>  
> - if (*ppos == mtd->size)
> + if (*ppos >= mtd->size)
>   return -ENOSPC;
>  
>   if (*ppos + count > mtd->size)



Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-18 Thread Boris Brezillon
On Sat,  7 Jul 2018 05:37:22 +0200
Jann Horn  wrote:

> The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> pread/pwrite syscalls bypass this.
> 
> I haven't found any codepath on which this actually causes dangerous
> behavior, but it seems like a sensible change anyway.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn 

Applied.

Thanks,

Boris

> ---
>  drivers/mtd/mtdchar.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index cd67c85cc87d..02389528f622 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> __user *buf, size_t count,
>  
>   pr_debug("MTD_read\n");
>  
> - if (*ppos + count > mtd->size)
> - count = mtd->size - *ppos;
> + if (*ppos + count > mtd->size) {
> + if (*ppos < mtd->size)
> + count = mtd->size - *ppos;
> + else
> + count = 0;
> + }
>  
>   if (!count)
>   return 0;
> @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> char __user *buf, size_t c
>  
>   pr_debug("MTD_write\n");
>  
> - if (*ppos == mtd->size)
> + if (*ppos >= mtd->size)
>   return -ENOSPC;
>  
>   if (*ppos + count > mtd->size)



Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-08 Thread Boris Brezillon
On Sat, 7 Jul 2018 11:03:00 +0200
Jann Horn  wrote:

> +cc linux-api
> 
> On Sat, Jul 7, 2018 at 10:44 AM Boris Brezillon
>  wrote:
> >
> > On Sat,  7 Jul 2018 05:37:22 +0200
> > Jann Horn  wrote:
> >  
> > > The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> > > `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> > > possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> > > wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> > > pread/pwrite syscalls bypass this.
> > >
> > > I haven't found any codepath on which this actually causes dangerous
> > > behavior, but it seems like a sensible change anyway.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn 
> > > ---
> > >  drivers/mtd/mtdchar.c | 10 +++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> > > index cd67c85cc87d..02389528f622 100644
> > > --- a/drivers/mtd/mtdchar.c
> > > +++ b/drivers/mtd/mtdchar.c
> > > @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> > > __user *buf, size_t count,
> > >
> > >   pr_debug("MTD_read\n");
> > >
> > > - if (*ppos + count > mtd->size)
> > > - count = mtd->size - *ppos;
> > > + if (*ppos + count > mtd->size) {
> > > + if (*ppos < mtd->size)
> > > + count = mtd->size - *ppos;
> > > + else
> > > + count = 0;
> > > + }  
> >
> > Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?  
> 
> Hmm, good question.
> The pread() manpage says that pread() can return the same errors as
> lseek(), and the lseek() manpage says that -EINVAL is for when you're
> trying to seek beyond the end of a seekable device. So from the
> documentation, it sounds as if you're right.
> But testing pread() beyond end of file for various things on my
> machine seems to just return 0:
> 
> # cat pread.c
> #include 
> #include 
> int main(int argc, char **argv) {
>   char buf[0x1000];
>   off_t off = strtoul(argv[1], NULL, 0);
>   pread(0, buf, 0x1000, off);
> }
> # gcc -o pread pread.c
> # strace -e trace=pread64 ./pread 2 < /dev/sda1
> pread64(0, "", 4096, 2) = 0
> +++ exited with 0 +++
> # strace -e trace=pread64 ./pread 10 <
> /sys/kernel/debug/x86/tlb_single_page_flush_ceiling
> pread64(0, "", 4096, 10)= 0
> +++ exited with 0 +++
> # strace -e trace=pread64 ./pread 200 < /dev/dm-2
> pread64(0, "", 4096, 200)   = 0
> +++ exited with 0 +++
> 
> Do you know of precedent for returning -EINVAL if *ppos is beyond the
> end of the device?

Nope, it just made more sense to me than returning 0. Anyway, let's
keep the most common behavior, even if it's not documented this way ;-).

> 
> > >
> > >   if (!count)
> > >   return 0;
> > > @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> > > char __user *buf, size_t c
> > >
> > >   pr_debug("MTD_write\n");
> > >
> > > - if (*ppos == mtd->size)
> > > + if (*ppos >= mtd->size)
> > >   return -ENOSPC;
> > >
> > >   if (*ppos + count > mtd->size)  
> >  
> 
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-08 Thread Boris Brezillon
On Sat, 7 Jul 2018 11:03:00 +0200
Jann Horn  wrote:

> +cc linux-api
> 
> On Sat, Jul 7, 2018 at 10:44 AM Boris Brezillon
>  wrote:
> >
> > On Sat,  7 Jul 2018 05:37:22 +0200
> > Jann Horn  wrote:
> >  
> > > The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> > > `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> > > possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> > > wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> > > pread/pwrite syscalls bypass this.
> > >
> > > I haven't found any codepath on which this actually causes dangerous
> > > behavior, but it seems like a sensible change anyway.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn 
> > > ---
> > >  drivers/mtd/mtdchar.c | 10 +++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> > > index cd67c85cc87d..02389528f622 100644
> > > --- a/drivers/mtd/mtdchar.c
> > > +++ b/drivers/mtd/mtdchar.c
> > > @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> > > __user *buf, size_t count,
> > >
> > >   pr_debug("MTD_read\n");
> > >
> > > - if (*ppos + count > mtd->size)
> > > - count = mtd->size - *ppos;
> > > + if (*ppos + count > mtd->size) {
> > > + if (*ppos < mtd->size)
> > > + count = mtd->size - *ppos;
> > > + else
> > > + count = 0;
> > > + }  
> >
> > Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?  
> 
> Hmm, good question.
> The pread() manpage says that pread() can return the same errors as
> lseek(), and the lseek() manpage says that -EINVAL is for when you're
> trying to seek beyond the end of a seekable device. So from the
> documentation, it sounds as if you're right.
> But testing pread() beyond end of file for various things on my
> machine seems to just return 0:
> 
> # cat pread.c
> #include 
> #include 
> int main(int argc, char **argv) {
>   char buf[0x1000];
>   off_t off = strtoul(argv[1], NULL, 0);
>   pread(0, buf, 0x1000, off);
> }
> # gcc -o pread pread.c
> # strace -e trace=pread64 ./pread 2 < /dev/sda1
> pread64(0, "", 4096, 2) = 0
> +++ exited with 0 +++
> # strace -e trace=pread64 ./pread 10 <
> /sys/kernel/debug/x86/tlb_single_page_flush_ceiling
> pread64(0, "", 4096, 10)= 0
> +++ exited with 0 +++
> # strace -e trace=pread64 ./pread 200 < /dev/dm-2
> pread64(0, "", 4096, 200)   = 0
> +++ exited with 0 +++
> 
> Do you know of precedent for returning -EINVAL if *ppos is beyond the
> end of the device?

Nope, it just made more sense to me than returning 0. Anyway, let's
keep the most common behavior, even if it's not documented this way ;-).

> 
> > >
> > >   if (!count)
> > >   return 0;
> > > @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> > > char __user *buf, size_t c
> > >
> > >   pr_debug("MTD_write\n");
> > >
> > > - if (*ppos == mtd->size)
> > > + if (*ppos >= mtd->size)
> > >   return -ENOSPC;
> > >
> > >   if (*ppos + count > mtd->size)  
> >  
> 
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-07 Thread Jann Horn
+cc linux-api

On Sat, Jul 7, 2018 at 10:44 AM Boris Brezillon
 wrote:
>
> On Sat,  7 Jul 2018 05:37:22 +0200
> Jann Horn  wrote:
>
> > The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> > `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> > possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> > wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> > pread/pwrite syscalls bypass this.
> >
> > I haven't found any codepath on which this actually causes dangerous
> > behavior, but it seems like a sensible change anyway.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn 
> > ---
> >  drivers/mtd/mtdchar.c | 10 +++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> > index cd67c85cc87d..02389528f622 100644
> > --- a/drivers/mtd/mtdchar.c
> > +++ b/drivers/mtd/mtdchar.c
> > @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> > __user *buf, size_t count,
> >
> >   pr_debug("MTD_read\n");
> >
> > - if (*ppos + count > mtd->size)
> > - count = mtd->size - *ppos;
> > + if (*ppos + count > mtd->size) {
> > + if (*ppos < mtd->size)
> > + count = mtd->size - *ppos;
> > + else
> > + count = 0;
> > + }
>
> Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?

Hmm, good question.
The pread() manpage says that pread() can return the same errors as
lseek(), and the lseek() manpage says that -EINVAL is for when you're
trying to seek beyond the end of a seekable device. So from the
documentation, it sounds as if you're right.
But testing pread() beyond end of file for various things on my
machine seems to just return 0:

# cat pread.c
#include 
#include 
int main(int argc, char **argv) {
  char buf[0x1000];
  off_t off = strtoul(argv[1], NULL, 0);
  pread(0, buf, 0x1000, off);
}
# gcc -o pread pread.c
# strace -e trace=pread64 ./pread 2 < /dev/sda1
pread64(0, "", 4096, 2) = 0
+++ exited with 0 +++
# strace -e trace=pread64 ./pread 10 <
/sys/kernel/debug/x86/tlb_single_page_flush_ceiling
pread64(0, "", 4096, 10)= 0
+++ exited with 0 +++
# strace -e trace=pread64 ./pread 200 < /dev/dm-2
pread64(0, "", 4096, 200)   = 0
+++ exited with 0 +++

Do you know of precedent for returning -EINVAL if *ppos is beyond the
end of the device?

> >
> >   if (!count)
> >   return 0;
> > @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> > char __user *buf, size_t c
> >
> >   pr_debug("MTD_write\n");
> >
> > - if (*ppos == mtd->size)
> > + if (*ppos >= mtd->size)
> >   return -ENOSPC;
> >
> >   if (*ppos + count > mtd->size)
>


Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-07 Thread Jann Horn
+cc linux-api

On Sat, Jul 7, 2018 at 10:44 AM Boris Brezillon
 wrote:
>
> On Sat,  7 Jul 2018 05:37:22 +0200
> Jann Horn  wrote:
>
> > The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> > `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> > possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> > wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> > pread/pwrite syscalls bypass this.
> >
> > I haven't found any codepath on which this actually causes dangerous
> > behavior, but it seems like a sensible change anyway.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn 
> > ---
> >  drivers/mtd/mtdchar.c | 10 +++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> > index cd67c85cc87d..02389528f622 100644
> > --- a/drivers/mtd/mtdchar.c
> > +++ b/drivers/mtd/mtdchar.c
> > @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> > __user *buf, size_t count,
> >
> >   pr_debug("MTD_read\n");
> >
> > - if (*ppos + count > mtd->size)
> > - count = mtd->size - *ppos;
> > + if (*ppos + count > mtd->size) {
> > + if (*ppos < mtd->size)
> > + count = mtd->size - *ppos;
> > + else
> > + count = 0;
> > + }
>
> Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?

Hmm, good question.
The pread() manpage says that pread() can return the same errors as
lseek(), and the lseek() manpage says that -EINVAL is for when you're
trying to seek beyond the end of a seekable device. So from the
documentation, it sounds as if you're right.
But testing pread() beyond end of file for various things on my
machine seems to just return 0:

# cat pread.c
#include 
#include 
int main(int argc, char **argv) {
  char buf[0x1000];
  off_t off = strtoul(argv[1], NULL, 0);
  pread(0, buf, 0x1000, off);
}
# gcc -o pread pread.c
# strace -e trace=pread64 ./pread 2 < /dev/sda1
pread64(0, "", 4096, 2) = 0
+++ exited with 0 +++
# strace -e trace=pread64 ./pread 10 <
/sys/kernel/debug/x86/tlb_single_page_flush_ceiling
pread64(0, "", 4096, 10)= 0
+++ exited with 0 +++
# strace -e trace=pread64 ./pread 200 < /dev/dm-2
pread64(0, "", 4096, 200)   = 0
+++ exited with 0 +++

Do you know of precedent for returning -EINVAL if *ppos is beyond the
end of the device?

> >
> >   if (!count)
> >   return 0;
> > @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> > char __user *buf, size_t c
> >
> >   pr_debug("MTD_write\n");
> >
> > - if (*ppos == mtd->size)
> > + if (*ppos >= mtd->size)
> >   return -ENOSPC;
> >
> >   if (*ppos + count > mtd->size)
>


Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-07 Thread Boris Brezillon
On Sat,  7 Jul 2018 05:37:22 +0200
Jann Horn  wrote:

> The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> pread/pwrite syscalls bypass this.
> 
> I haven't found any codepath on which this actually causes dangerous
> behavior, but it seems like a sensible change anyway.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn 
> ---
>  drivers/mtd/mtdchar.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index cd67c85cc87d..02389528f622 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> __user *buf, size_t count,
>  
>   pr_debug("MTD_read\n");
>  
> - if (*ppos + count > mtd->size)
> - count = mtd->size - *ppos;
> + if (*ppos + count > mtd->size) {
> + if (*ppos < mtd->size)
> + count = mtd->size - *ppos;
> + else
> + count = 0;
> + }

Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?

>  
>   if (!count)
>   return 0;
> @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> char __user *buf, size_t c
>  
>   pr_debug("MTD_write\n");
>  
> - if (*ppos == mtd->size)
> + if (*ppos >= mtd->size)
>   return -ENOSPC;
>  
>   if (*ppos + count > mtd->size)



Re: [PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-07 Thread Boris Brezillon
On Sat,  7 Jul 2018 05:37:22 +0200
Jann Horn  wrote:

> The first checks in mtdchar_read() and mtdchar_write() attempt to limit
> `count` such that `*ppos + count <= mtd->size`. However, they ignore the
> possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
> wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
> pread/pwrite syscalls bypass this.
> 
> I haven't found any codepath on which this actually causes dangerous
> behavior, but it seems like a sensible change anyway.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn 
> ---
>  drivers/mtd/mtdchar.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index cd67c85cc87d..02389528f622 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char 
> __user *buf, size_t count,
>  
>   pr_debug("MTD_read\n");
>  
> - if (*ppos + count > mtd->size)
> - count = mtd->size - *ppos;
> + if (*ppos + count > mtd->size) {
> + if (*ppos < mtd->size)
> + count = mtd->size - *ppos;
> + else
> + count = 0;
> + }

Hm, shouldn't we return -ERANGE or -EINVAL if *ppos >= mtd->size?

>  
>   if (!count)
>   return 0;
> @@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const 
> char __user *buf, size_t c
>  
>   pr_debug("MTD_write\n");
>  
> - if (*ppos == mtd->size)
> + if (*ppos >= mtd->size)
>   return -ENOSPC;
>  
>   if (*ppos + count > mtd->size)



[PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-06 Thread Jann Horn
The first checks in mtdchar_read() and mtdchar_write() attempt to limit
`count` such that `*ppos + count <= mtd->size`. However, they ignore the
possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
pread/pwrite syscalls bypass this.

I haven't found any codepath on which this actually causes dangerous
behavior, but it seems like a sensible change anyway.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn 
---
 drivers/mtd/mtdchar.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index cd67c85cc87d..02389528f622 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char __user 
*buf, size_t count,
 
pr_debug("MTD_read\n");
 
-   if (*ppos + count > mtd->size)
-   count = mtd->size - *ppos;
+   if (*ppos + count > mtd->size) {
+   if (*ppos < mtd->size)
+   count = mtd->size - *ppos;
+   else
+   count = 0;
+   }
 
if (!count)
return 0;
@@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const char 
__user *buf, size_t c
 
pr_debug("MTD_write\n");
 
-   if (*ppos == mtd->size)
+   if (*ppos >= mtd->size)
return -ENOSPC;
 
if (*ppos + count > mtd->size)
-- 
2.18.0.203.gfac676dfb9-goog



[PATCH] mtdchar: fix overflows in adjustment of `count`

2018-07-06 Thread Jann Horn
The first checks in mtdchar_read() and mtdchar_write() attempt to limit
`count` such that `*ppos + count <= mtd->size`. However, they ignore the
possibility of `*ppos > mtd->size`, allowing the calculation of `count` to
wrap around. `mtdchar_lseek()` prevents seeking beyond mtd->size, but the
pread/pwrite syscalls bypass this.

I haven't found any codepath on which this actually causes dangerous
behavior, but it seems like a sensible change anyway.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn 
---
 drivers/mtd/mtdchar.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index cd67c85cc87d..02389528f622 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -160,8 +160,12 @@ static ssize_t mtdchar_read(struct file *file, char __user 
*buf, size_t count,
 
pr_debug("MTD_read\n");
 
-   if (*ppos + count > mtd->size)
-   count = mtd->size - *ppos;
+   if (*ppos + count > mtd->size) {
+   if (*ppos < mtd->size)
+   count = mtd->size - *ppos;
+   else
+   count = 0;
+   }
 
if (!count)
return 0;
@@ -246,7 +250,7 @@ static ssize_t mtdchar_write(struct file *file, const char 
__user *buf, size_t c
 
pr_debug("MTD_write\n");
 
-   if (*ppos == mtd->size)
+   if (*ppos >= mtd->size)
return -ENOSPC;
 
if (*ppos + count > mtd->size)
-- 
2.18.0.203.gfac676dfb9-goog