linux-kernel - Re: [PATCH] fs: fix i_writecount on shmem and friends

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 12 Mar 2014 18:19:25 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	David Herrmann <dh.herrmann@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Howells <dhowells@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>,
	stable <stable@...r.kernel.org>, Neil Brown <neilb@...e.de>
Subject: Re: [PATCH] fs: fix i_writecount on shmem and friends

On Tue, Mar 11, 2014 at 12:05:09PM -0700, Linus Torvalds wrote:
> 
> which returns ETXTBSY (most easily seen by just stracing it).
> 
> The patch would also seem to make sense, with the i_readcount_inc()
> being immediately below for the FMODE_READ case.

I think it's trying to fix the problem in the wrong place.  The bug is real,
all right, but it's not that alloc_file() for non-regulars doesn't grab
writecount; it's that drop_file_write_access() drops it for those.

What the hell would we want to play with that counter for, anyway?  It's not
as if they could be mmapped, so all it does is making pipe(2) and socket(2)
more costly, for no visible reason.

I would prefer to flip
        put_write_access(inode);

        if (special_file(inode->i_mode))
                return;
in drop_file_write_access() instead.

<goes to looks at i_writecount users>
Oh, shit...

drivers/md/md.c:
/* similar to deny_write_access, but accounts for our holding a reference
 * to the file ourselves */
static int deny_bitmap_write_access(struct file * file)
{
        struct inode *inode = file->f_mapping->host;

        spin_lock(&inode->i_lock);
        if (atomic_read(&inode->i_writecount) > 1) {
                spin_unlock(&inode->i_lock);
                return -ETXTBSY;
        }
        atomic_set(&inode->i_writecount, -1);
        spin_unlock(&inode->i_lock);

        return 0;
}

Broken.  get_write_access() will happily increment i_writecount e.g. from
1 to 2, without even looking at i_lock.  Moreover, it's paired with
void restore_bitmap_write_access(struct file *file)
{
        struct inode *inode = file->f_mapping->host;

        spin_lock(&inode->i_lock);
        atomic_set(&inode->i_writecount, 1);
        spin_unlock(&inode->i_lock);
}
Just what will happen if we do denywrite mmap() of that file in between?
Even worse, the caller take file straight from fget(), with no sanity
checks whatsoever.  Just what will happen if I give it e.g. a directory?
Or a procfs/sysfs/whatnot file, for that matter?  Neil?  I realize that
it's root-only, but still...
--
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/