| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 9 Feb 2009 19:48:25 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: "David VomLehn (dvomlehn)" <dvomlehn@...co.com>
Cc: <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Propagate CRAMFS uncompression errors
On Mon, 9 Feb 2009 22:28:15 -0500 "David VomLehn (dvomlehn)" <dvomlehn@...co.com> wrote:
>
> > From: Andrew Morton [mailto:akpm@...ux-foundation.org]
> > Sent: Monday, February 09, 2009 3:17 PM
> > Subject: Re: [PATCH] Propagate CRAMFS uncompression errors
> >
> > "David VomLehn (dvomlehn)" <dvomlehn@...co.com> wrote:
> >
> > > If cramfs_uncompress_block detects an error uncompressing it will
> > > return a zero value. This patch checks the return value and
> > propagates
> > > the error back up to the block layer.
> > >
> > > Signed-off-by: David VomLehn <dvomlehn@...co.com>
> > > ---
> > > fs/cramfs/inode.c | 10 +++++++++-
> > > 1 files changed, 9 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > > index a07338d..6ff8a5e 100644
> > > --- a/fs/cramfs/inode.c
> > > +++ b/fs/cramfs/inode.c
> > > @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file,
> > > struct page * page)
> >
> > Your email client is wordwrapping the patches.
>
> Sorry. I sent it to myself first, and didn't have a problem. I hate
> email programs.
>
> > > memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE -
> > > bytes_filled);
> > > kunmap(page);
> > > flush_dcache_page(page);
> > > - SetPageUptodate(page);
> > > +
> > > + if (bytes_filled == 0) {
> > > + ClearPageUptodate(page);
> > > + SetPageError(page);
> > > + }
> > > +
> > > + else
> > > + SetPageUptodate(page);
> > > +
> > > unlock_page(page);
> > > return 0;
> >
> > A more typical code layout would be
> >
> > if (bytes_filled == 0) {
> > ClearPageUptodate(page);
> > SetPageError(page);
> > } else
> > SetPageUptodate(page);
> >
> > or (better, IMO):
> >
> > if (bytes_filled == 0) {
> > ClearPageUptodate(page);
> > SetPageError(page);
> > } else {
> > SetPageUptodate(page);
> > }
> >
> >
> > This patch will incorrectly cause the driver to report an IO error if
> > the (page->index < maxblock) test returns false. For example, a
> > pread() which is wholly outside the end-of-file should return
> > zero, not
> > -EIO.
> >
> > cramfs_readpage() handles this case very strangely, although not
> > obviously buggily. Probably this function never even gets
> > called for a
> > read wholly outside i_size.
> >
> > How does this version look to you?
> >
> > --- a/fs/cramfs/inode.c~propagate-cramfs-uncompression-errors
> > +++ a/fs/cramfs/inode.c
> > @@ -488,12 +488,19 @@ static int cramfs_readpage(struct file *
> > compr_len);
> > mutex_unlock(&read_mutex);
> > }
> > - } else
> > - pgdata = kmap(page);
> > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE -
> > bytes_filled);
> > - kunmap(page);
> > - flush_dcache_page(page);
> > - SetPageUptodate(page);
> > +
> > + if (bytes_filled == 0) {
> > + /* Decompression error */
> > + ClearPageUptodate(page);
> > + SetPageError(page);
> > + } else {
> > + memset(pgdata + bytes_filled, 0,
> > + PAGE_CACHE_SIZE - bytes_filled);
> > + flush_dcache_page(page);
> > + SetPageUptodate(page);
> > + }
> > + kunmap(page);
> > + }
> > unlock_page(page);
> > return 0;
> > }
>
> Better, thanks!
umm...
Nope, it's still not right. We'll treat this case:
if (compr_len == 0)
; /* hole */
as an IO error. grr.
--- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors
+++ a/fs/cramfs/inode.c
@@ -477,7 +477,7 @@ static int cramfs_readpage(struct file *
mutex_unlock(&read_mutex);
pgdata = kmap(page);
if (compr_len == 0)
- ; /* hole */
+ goto out; /* hole */
else if (compr_len > (PAGE_CACHE_SIZE << 1))
printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len);
else {
@@ -488,12 +488,20 @@ static int cramfs_readpage(struct file *
compr_len);
mutex_unlock(&read_mutex);
}
- } else
- pgdata = kmap(page);
- memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled);
- kunmap(page);
- flush_dcache_page(page);
- SetPageUptodate(page);
+
+ if (bytes_filled == 0) {
+ /* Decompression error */
+ ClearPageUptodate(page);
+ SetPageError(page);
+ } else {
+ memset(pgdata + bytes_filled, 0,
+ PAGE_CACHE_SIZE - bytes_filled);
+ flush_dcache_page(page);
+ SetPageUptodate(page);
+ }
+ kunmap(page);
+ }
+out:
unlock_page(page);
return 0;
}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists