linux-ext4 - Re: [PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP

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]

Message-ID: <20130407194855.GA5292@thunk.org>
Date:	Sun, 7 Apr 2013 15:48:55 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	"Dr. Tilmann Bubeck" <t.bubeck@...nform.de>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH -v3] ext4: implementation of a new ioctl called
 EXT4_IOC_SWAP_BOOT

On Sun, Apr 07, 2013 at 11:09:58AM -0600, Andreas Dilger wrote:
> > +    filemap_flush(inode_bl->i_mapping);
> 
> Technically, it shouldn't be possible to have dirty pages on the
> bootloader inode, since any inode swapped there will itself have
> been flushed, and should not be directly accessible after that
> point. That said, I guess this ioctl() won't be called too often so
> the empty flush is mostly harmless.

Yes, technically there should be no mappings for the inode boot
loader.  (Well, unless someone had one of the unofficial open-by-inode
number patches).

> > +    if (inode_bl->i_nlink == 0) {
> > +        /* this inode has never been used as a BOOT_LOADER */
> > +        set_nlink(inode_bl, 1);
> > +        i_uid_write(inode_bl, 0);
> > +        i_gid_write(inode_bl, 0);
> > +        inode_bl->i_flags = 0;
> > +        ei_bl->i_flags = 0;
> > +        inode_bl->i_version = 1;
> > +        i_size_write(inode_bl, 0);
> > +        inode_bl->i_mode = S_IFREG;
> > +        if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> > +                          EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> > +            ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
> > +            ext4_ext_tree_init(handle, inode_bl);
> > +        } else
> > +            memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));
> 
> I don't understand this. Wouldn't this clobber the block pointers if 
> an existing boot inode and cause them to leak?  This seems broken to me. 

If i_nlink is zero, then there is no existing boot loader inode.  In
theory the rest of the inode is zero, but this is to make sure the
inode is initialized to something sane before we swap it into the
user-visible inode.

I'll fix up the rest of the minor errors and typos which you pointed
out, thanks!

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html