linux-kernel - Re: [git pull] mnt

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:	Thu, 17 Mar 2011 10:44:34 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Xiaotian Feng <xtfeng@...il.com>
Cc:	tony.luck@...el.com,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] mnt_devname queue

On Thu, Mar 17, 2011 at 03:28:47PM +0800, Xiaotian Feng wrote:
> Cc'ed author ...
> 
> > a) What the hell would you expect to happen if userland mounts it twice
> > and unmount the first one? ??pstore_sb = NULL, pstore_mnt = NULL, AFAICS.

BTW, you want
	* mount_single(), not mount_nodev()
	* simple_pin_fs()/mntput() around modifying that sucker from kernel
(held across both the file creation and writing to it)

> > b) pstore_writefile() - struct file on stack? ??Really? ??Again, in the
> > scenario above, what'll happen to you if pstore_mnt gets dropped and
> > freed in the middle of all that?
> >
> > c) in the same function:
> > + ?? ?? ?? memset(&f, '0', sizeof f);
> > Ahem...

Aside of a new meaning given to "zero that structure out", why the devil
are you doing it in such a convoluted way?  Note that you are using ramfs,
so the mapping is unevictable.  Simple kmalloc() + memcpy() and
simple_read_from_buffer() to implement ->read() would do nicely.

And for fsck sake, copy ramfs_get_inode() and trim it.  You are overriding
it for the single directory in there and you are only using it for regular
files otherwise.  With wrong ->i_op and ->i_fop *and* irrelevant messing with
->i_mapping.  Which leaves you with
	inode->i_ino = get_next_ino();
	inode_init_owner(inode, dir, mode);
	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
as useful part.

BTW, you want to make the contents visible before d_add().  You
*definitely* want to finish setting it up before unlocking the parent,
or you are asking for userland to come and unlink() it under you.

Incidentally, you are leaking ->i_private on umount.  You want ->evict_inode()
doing that kfree(), not ->unlink().  And possibly ->erase() as well, with
check for zero i_nlink around it (in ->evict_inode()).
--
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/