Date: Mon, 19 Jan 2009 22:09:28 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Stanislav Sedov <stas@freebsd.org> Cc: Kostik Belousov <kostikbel@gmail.com>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r187395 - head/sys/gnu/fs/ext2fs Message-ID: <20090119202452.X37158@delplex.bde.org> In-Reply-To: <20090118174357.3ed1459d.stas@FreeBSD.org> References: <200901181404.n0IE4uXw075698@svn.freebsd.org> <20090118140644.GT48057@deviant.kiev.zoral.com.ua> <20090118174357.3ed1459d.stas@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 18 Jan 2009, Stanislav Sedov wrote:
> Kostik Belousov <kostikbel@gmail.com> mentioned:
>
>>
>> Please see a discussion on the fs@ and reasoning why I declined to commit
>> the similar patch.
> ...
> The extra size added in inodes are used to store additional info like extended
> attributes, ACLs and so on. Each inode now has a field just after the legacy
> inode format struct that shows hom much additional space has been added to
> this particular inode. By analyzing this field the operating system can interpret
> additional data located in inode, if it understand its format (there might be
> application and/or OS specific data too).
>
> Our implementation just ignore this additional fields after sizeof(ext2fs_inode),
> both while reading and writing. Thus we don't interet this data yet we don't
> overwrite it.
If it does this, then it is quite broken, since the garbage data bites
implementations that _do_ interpret the data. Writing to the unsupported
fields is especially important for either destruction or creation, so that
the garbage doesn't leak to new files when a disk inode is reused.
The field[s] showing how the additional space is used actually seem to be
_inside_ the legacy inode (*). However, this doesn't seem to help, since
we seem to never touch them there either (we bzero() new inodes in
ext2_vget() but never bzero() the disk inode). There seem to be old
bugs in this area. The untouched fields are:
- osd1 (sic) I don't understand this
- i_file_acl leaking this would security holes
- i_dir_acl leaking this would security holes
this is abused for the high 32 bits of the file size in the
case of regular files. We use it in this case, but a few
years ago when we didn't use it, leaking it would have
given wrong file sizes.
- i_faddr I don't understand this
_ osd2 (sic) this has mainly things like an extra 16 bits for the the uid
and gid. Leaking these would give security holes more often
than for acls.
I think there is no actual problem with at least the file acls. What is
supposed to happen is:
- if the file system has any nonzero acls, then ext[23]fs under linux has
set EXT2_FEATURE_COMPAT_EXT_ATTR in the superblock to indicate that
this feature is used
- FreeBSD ext2fs doesn't support this feature, so mounting such file systems
fails
- an ext2fs utility should offer to clear EXT2_FEATURE_COMPAT_EXT_ATTR in the
superblock. When it clears it there, it must clear all the associated
metadata too. Thus the metadata doesn't leak.
- hopefully there are compat flags for all the other extensions too. There
is one for using the file size extension. I can't find one related to
larger disk inodes.
(*) In linux-2.6.10, the declaration of struct ext3_inode is lexically
identical to struct ext2_inode except for one s/2/3/ and apparent
style bugs in the latter (__u16/32 hasn't been converted to __le16/32
in new fields in the latter; this is only a style bug provided the
new fields are ignored on read and zeroed on write).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090119202452.X37158>