| 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 21:10:49 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: dvomlehn@...co.com
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Propagate CRAMFS uncompression errors
On Mon, 09 Feb 2009 20:06:01 -0800 David VomLehn <dvomlehn@...co.com> wrote:
>
> On Mon, 2009-02-09 at 19:48 -0800, Andrew Morton wrote:
> ...
> >
> > 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;
> > }
>
> Is that actually an error. The comment in the code is, uh, a bit terse,
> but I took it to mean that there was a deliberate hole, just like holes
> in files that should read as all zeroes. But even if it's actually an
> error, we still need to kunmap the page
Yes, we need to unmap the page.
It's better to perform those checks before mapping it.
--- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors
+++ a/fs/cramfs/inode.c
@@ -475,25 +475,32 @@ static int cramfs_readpage(struct file *
start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
mutex_unlock(&read_mutex);
- pgdata = kmap(page);
if (compr_len == 0)
- ; /* hole */
- else if (compr_len > (PAGE_CACHE_SIZE << 1))
- printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len);
- else {
- mutex_lock(&read_mutex);
- bytes_filled = cramfs_uncompress_block(pgdata,
- PAGE_CACHE_SIZE,
- cramfs_read(sb, start_offset, compr_len),
- compr_len);
- mutex_unlock(&read_mutex);
+ goto out; /* hole */
+ if (compr_len > (PAGE_CACHE_SIZE << 1)) {
+ printk(KERN_ERR "cramfs: bad compressed blocksize %u\n",
+ compr_len);
+ goto out;
}
- } else
pgdata = kmap(page);
- memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled);
- kunmap(page);
- flush_dcache_page(page);
- SetPageUptodate(page);
+ mutex_lock(&read_mutex);
+ bytes_filled = cramfs_uncompress_block(pgdata, PAGE_CACHE_SIZE,
+ cramfs_read(sb, start_offset, compr_len), compr_len);
+ mutex_unlock(&read_mutex);
+
+ 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;
}
But if address_space_operations.readpage() encounters a hole, it is
supposed to return zeroes to userspace (see, for example,
block_read_full_page()'s !buffer_mapped() handling).
Sigh. That code needs more thought than I am apparently able to give it :(
--
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