linux-kernel - Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

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, 05 Aug 2009 10:21:26 -0300
From:	Ramon de Carvalho Valle <ramon@...esecurity.org>
To:	Eric Sandeen <sandeen@...deen.net>
Cc:	Christoph Hellwig <hch@...radead.org>, xfs@....sgi.com, hch@....de,
	linux-kernel@...r.kernel.org, mszeredi@...ell.com
Subject: Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer
 check

On Tue, 2009-08-04 at 23:15 -0500, Eric Sandeen wrote:
> Ramon de Carvalho Valle wrote:
> > On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
> >> Ramon de Carvalho Valle wrote:
> >>> The xfs_iformat function does not check if the realtime device target pointer
> >>> is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode
> >>> structure.
> >>>
> >>> Signed-off-by: Christoph Hellwig <hch@...radead.org>
> >>> Signed-off-by: Ramon de Carvalho Valle <ramon@...esecurity.org>
> >>> Cc: stable <stable@...nel.org>
> >>> ---
> >>>  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
> >>>  1 files changed, 17 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> >>> index 1f22d65..37d3ee5 100644
> >>> --- a/fs/xfs/xfs_inode.c
> >>> +++ b/fs/xfs/xfs_inode.c
> >>> @@ -343,13 +343,24 @@ xfs_iformat(
> >>>  		return XFS_ERROR(EFSCORRUPTED);
> >>>  	}
> >>>  
> >>> +	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
> >>> +			!ip->i_mount->m_rtdev_targp)) {
> >>> +		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
> >>> +			"corrupt dinode %Lu, flags = 0x%x.",
> >>> +			(unsigned long long)ip->i_ino,
> >>> +			ip->i_d.di_flags);
> >>> +		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
> >>> +				     ip->i_mount, dip);
> >> I think I'd rather not change all the corruption text tag ordering;
> >> it'll make it harder to track down any common occurrences of
> >> "xfs_iformat(X)" corruption in the future if they get renumbered now.
> >>
> >> I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph
> >> had.  "realtime" is a lot more informative than "3" anyway.
> > 
> > I don't think this is a bad decision, because the corruption errors can
> > be easily identified by the output of xfs_fs_repair_cmn_err and the
> > source line. I think this is a reasonable change that will keep the code
> > clean and consistent.
> 
> Until you wind up looking at a problem from some old kernel, or modified
> vendor kernel, and you realize that now you really don't know which
> error "xfs_iformat(6)" is anymore, and you either have to go digging
> through trees that aren't handy, or you just give up and don't bother to
> help because now it's too much of a pain.  ;)
> 
> But I can leave it up to the folks @ sgi, I can see both sides of the
> argument, and I won't care too much either way.

Yes, whatever they decide should be ok. Thanks for your feedback Eric.

-Ramon

> 
> Thanks,
> -Eric
> 
> > -Ramon
> > 
> >> -Eric
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)