ext2fs_block_iterate() on fast symlink

ext2fs_block_iterate() on fast symlink

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

  Hello,

  when  ext2fs_block_iterate() is called on a fast symlink (and I assume
device inodes would be no different), then random things happen - the
problem is ext2fs_block_iterate() just blindly takes portions of the inode
and treats them as block numbers. Now I agree that garbage went in (it
makes no sence to call this function on such inode) so garbage results but
maybe it would be nicer to handle it more gracefully. Attached patch should
do it.

									Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

[-- Attachment #2: e2fsprogs-block_iterate_fix.diff --]
[-- Type: text/x-patch, Size: 878 bytes --]

--- a/lib/ext2fs/inode.c	2007-06-20 13:55:52.000000000 +0200
+++ b/lib/ext2fs/inode.c	2007-06-20 14:11:15.000000000 +0200
@@ -771,6 +771,10 @@ errcode_t ext2fs_get_blocks(ext2_filsys 
 	retval = ext2fs_read_inode(fs, ino, &inode);
 	if (retval)
 		return retval;
+	if (LINUX_S_ISCHR(inode.i_mode) || LINUX_S_ISBLK(inode.i_mode) ||
+	    (LINUX_S_ISLNK(inode.i_mode) &&
+	     ext2fs_inode_data_blocks(fs, &inode) == 0))
+		return EXT2_ET_INVAL_INODE_TYPE;
 	for (i=0; i < EXT2_N_BLOCKS; i++)
 		blocks[i] = inode.i_block[i];
 	return 0;
--- a/lib/ext2fs/ext2_err.et.in	2007-06-20 14:09:18.000000000 +0200
+++ b/lib/ext2fs/ext2_err.et.in	2007-06-20 14:11:25.000000000 +0200
@@ -296,5 +296,8 @@ ec	EXT2_ET_RESIZE_INODE_CORRUPT,
 ec	EXT2_ET_SET_BMAP_NO_IND,
 	"Missing indirect block not present"
 
+ec	EXT2_ET_INVAL_INODE_TYPE,
+	"Invalid inode type for the operation."
+
 	end
 

Re: ext2fs_block_iterate() on fast symlink

[Andreas Dilger]

On Jun 20, 2007  14:56 +0200, Jan Kara wrote:
>   when  ext2fs_block_iterate() is called on a fast symlink (and I assume
> device inodes would be no different), then random things happen - the
> problem is ext2fs_block_iterate() just blindly takes portions of the inode
> and treats them as block numbers. Now I agree that garbage went in (it
> makes no sence to call this function on such inode) so garbage results but
> maybe it would be nicer to handle it more gracefully. Attached patch should
> do it.

> --- a/lib/ext2fs/inode.c	2007-06-20 13:55:52.000000000 +0200
> +++ b/lib/ext2fs/inode.c	2007-06-20 14:11:15.000000000 +0200
> @@ -771,6 +771,10 @@ errcode_t ext2fs_get_blocks(ext2_filsys 
>  	retval = ext2fs_read_inode(fs, ino, &inode);
>  	if (retval)
>  		return retval;
> +	if (LINUX_S_ISCHR(inode.i_mode) || LINUX_S_ISBLK(inode.i_mode) ||
> +	    (LINUX_S_ISLNK(inode.i_mode) &&
> +	     ext2fs_inode_data_blocks(fs, &inode) == 0))
> +		return EXT2_ET_INVAL_INODE_TYPE;

I would prefer that we NOT continue to make fast symlinks conditional upon
the i_blocks count.  That causes problems if e.g. an EA block is present
(that would cause this blocks == 0 test to incorrectly fail), and may making
the check (blocks - !!i_file_acl) can still fail for other reasons where a
block is added to an inode (e.g. if we have larger EAs, etc).

I'd prefer to make this check "i_size < sizeof(i_block)" or similar, which
has always been true for fast symlinks, for every kernel that I have ever
seen.


Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: ext2fs_block_iterate() on fast symlink

[Jan Kara]

On Thu 21-06-07 03:33:43, Andreas Dilger wrote:
> On Jun 20, 2007  14:56 +0200, Jan Kara wrote:
> >   when  ext2fs_block_iterate() is called on a fast symlink (and I assume
> > device inodes would be no different), then random things happen - the
> > problem is ext2fs_block_iterate() just blindly takes portions of the inode
> > and treats them as block numbers. Now I agree that garbage went in (it
> > makes no sence to call this function on such inode) so garbage results but
> > maybe it would be nicer to handle it more gracefully. Attached patch should
> > do it.
> 
> > --- a/lib/ext2fs/inode.c	2007-06-20 13:55:52.000000000 +0200
> > +++ b/lib/ext2fs/inode.c	2007-06-20 14:11:15.000000000 +0200
> > @@ -771,6 +771,10 @@ errcode_t ext2fs_get_blocks(ext2_filsys 
> >  	retval = ext2fs_read_inode(fs, ino, &inode);
> >  	if (retval)
> >  		return retval;
> > +	if (LINUX_S_ISCHR(inode.i_mode) || LINUX_S_ISBLK(inode.i_mode) ||
> > +	    (LINUX_S_ISLNK(inode.i_mode) &&
> > +	     ext2fs_inode_data_blocks(fs, &inode) == 0))
> > +		return EXT2_ET_INVAL_INODE_TYPE;
> 
> I would prefer that we NOT continue to make fast symlinks conditional upon
> the i_blocks count.  That causes problems if e.g. an EA block is present
> (that would cause this blocks == 0 test to incorrectly fail), and may making
> the check (blocks - !!i_file_acl) can still fail for other reasons where a
> block is added to an inode (e.g. if we have larger EAs, etc).
  Note that ext2fs_inode_data_blocks() subtract number of EA blocks, so it
is equivalent to (blocks - !!i_file_acl). The function is supposed to
return the number of real data blocks so the test should be fine even in
future.

> I'd prefer to make this check "i_size < sizeof(i_block)" or similar, which
> has always been true for fast symlinks, for every kernel that I have ever
> seen.
  Personally I don't mind much. If Ted finds this better, I'll change that.
Maybe introducing some macro LINUX_S_ISFASTLNK() would be fine.

									Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: ext2fs_block_iterate() on fast symlink

[Andreas Dilger]

On Jun 21, 2007  11:54 +0200, Jan Kara wrote:
> On Thu 21-06-07 03:33:43, Andreas Dilger wrote:
> > > +	if (LINUX_S_ISCHR(inode.i_mode) || LINUX_S_ISBLK(inode.i_mode) ||
> > > +	    (LINUX_S_ISLNK(inode.i_mode) &&
> > > +	     ext2fs_inode_data_blocks(fs, &inode) == 0))
> > > +		return EXT2_ET_INVAL_INODE_TYPE;
> > 
> > I would prefer that we NOT continue to make fast symlinks conditional upon
> > the i_blocks count.  That causes problems if e.g. an EA block is present
> > (that would cause this blocks == 0 test to incorrectly fail), and may making
> > the check (blocks - !!i_file_acl) can still fail for other reasons where a
> > block is added to an inode (e.g. if we have larger EAs, etc).
>
>   Note that ext2fs_inode_data_blocks() subtract number of EA blocks, so it
> is equivalent to (blocks - !!i_file_acl). The function is supposed to
> return the number of real data blocks so the test should be fine even in
> future.

This is where I disagree.  We had a whole series of bugs in different places
when SELinux patched ext2/3 to allow EAs on symlinks (also breaking ext2/3
compatibility in the process), because fast symlink checking was based on
i_blocks and not i_size.  Then we fixed those bugs, but we are open to a
whole series of new bugs should any other blocks be assigned to an symlink.

> > I'd prefer to make this check "i_size < sizeof(i_block)" or similar, which
> > has always been true for fast symlinks, for every kernel that I have ever
> > seen.
>
>   Personally I don't mind much. If Ted finds this better, I'll change that.
> Maybe introducing some macro LINUX_S_ISFASTLNK() would be fine.

Ted, are you aware of any kernel that ever wrote a short symlink into an
external block?  Since it isn't possible to modify a symlink after it is
created (except adding an EA ;-) it should never be possible that a "slow"
symlink is shortened but left in the external block.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.