* [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode @ 2017-01-11 8:24 Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 1/6] xfs: make the ASSERT() condition likely Amir Goldstein ` (5 more replies) 0 siblings, 6 replies; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs Darrick, This is 8th revision of the fixes for malformed on-disk i_mode. Patches 1-2 are reviewed by Christoph and IMO, you should consider patch 1 and perhaprs also patch 2 for 4.10 and stable 4.9. Patches 4-6 replace xfs_mode_to_ftype table with switch statement per Christoph's suggestion and add sanity checks. Tested with generic/401 with -n ftype=0|1. Tested with new xfs/348 test with -n ftype=0|1. Amir. v8: - Address review comments by Darrick - Added Reviewed-by Darrick to patches 4,6 v7: - Replaced xfs_mode_to_ftype table with switch statement - Reordered patches so reviewed trivial patches are first - Added Reviewed-by Christoph to first 2 patches - Added cleanup patch for xfs_dir2.h - Added sanity checks for invalid mode in more places v6: - Added Reviewed-by Brian for patch 1 - Added patch to address new xfs/348 failures - Added patch to fix ASSERT() likely v5: - remove wrong argument about on-disk malformed mode from commit message - address Brian's review comments v4: - independent fix patch for xfs *** BLURB HERE *** Amir Goldstein (6): xfs: make the ASSERT() condition likely xfs: sanity check directory inode di_size xfs: add missing include dependencies to xfs_dir2.h xfs: replace xfs_mode_to_ftype table with switch statement xfs: sanity check inode mode when creating new dentry xfs: sanity check inode di_mode fs/xfs/libxfs/xfs_dir2.c | 39 ++++++++++++++++++++------------- fs/xfs/libxfs/xfs_dir2.h | 8 ++++--- fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++++-- fs/xfs/xfs_iops.c | 50 ++++++++++++++++++++++++++++++++++--------- fs/xfs/xfs_linux.h | 6 +++--- 5 files changed, 80 insertions(+), 33 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 1/6] xfs: make the ASSERT() condition likely 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-13 7:25 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 2/6] xfs: sanity check directory inode di_size Amir Goldstein ` (4 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs The ASSERT() condition is the normal case, not the exception, so testing the condition should be likely(), not unlikely(). Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/xfs_linux.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index e467218..7a989de 100644 --- a/fs/xfs/xfs_linux.h +++ b/fs/xfs/xfs_linux.h @@ -331,11 +331,11 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y) } #define ASSERT_ALWAYS(expr) \ - (unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) + (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) #ifdef DEBUG #define ASSERT(expr) \ - (unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) + (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) #ifndef STATIC # define STATIC noinline @@ -346,7 +346,7 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y) #ifdef XFS_WARN #define ASSERT(expr) \ - (unlikely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__)) + (likely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__)) #ifndef STATIC # define STATIC static noinline -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 1/6] xfs: make the ASSERT() condition likely 2017-01-11 8:24 ` [PATCH v8 1/6] xfs: make the ASSERT() condition likely Amir Goldstein @ 2017-01-13 7:25 ` Christoph Hellwig 2017-01-18 20:57 ` Darrick J. Wong 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2017-01-13 7:25 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 10:24:05AM +0200, Amir Goldstein wrote: > The ASSERT() condition is the normal case, not the exception, > so testing the condition should be likely(), not unlikely(). Btw, I think at least this one is 4.10-rc material, although the others in this series might be candidates as well. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 1/6] xfs: make the ASSERT() condition likely 2017-01-13 7:25 ` Christoph Hellwig @ 2017-01-18 20:57 ` Darrick J. Wong 0 siblings, 0 replies; 14+ messages in thread From: Darrick J. Wong @ 2017-01-18 20:57 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Amir Goldstein, Brian Foster, linux-xfs On Fri, Jan 13, 2017 at 08:25:04AM +0100, Christoph Hellwig wrote: > On Wed, Jan 11, 2017 at 10:24:05AM +0200, Amir Goldstein wrote: > > The ASSERT() condition is the normal case, not the exception, > > so testing the condition should be likely(), not unlikely(). > > Btw, I think at least this one is 4.10-rc material, although the others > in this series might be candidates as well. They are, I can crash the kernel with a zero-length directory, which one of the patches in this series fixes. Applied, soaking into for-next now. --D > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 2/6] xfs: sanity check directory inode di_size 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 1/6] xfs: make the ASSERT() condition likely Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-14 6:28 ` Darrick J. Wong 2017-01-11 8:24 ` [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein ` (3 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs This changes fixes an assertion hit when fuzzing on-disk i_mode values. The easy case to fix is when changing an empty file i_mode to S_IFDIR. In this case, xfs_dinode_verify() detects an illegal zero size for directory and fails to load the inode structure from disk. For the case of non empty file whose i_mode is changed to S_IFDIR, the ASSERT() statement in xfs_dir2_isblock() is replaced with return -EFSCORRUPTED, to avoid interacting with corrupted jusk also when XFS_DEBUG is disabled. Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/libxfs/xfs_dir2.c | 3 ++- fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index c58d72c..4f7913f 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -631,7 +631,8 @@ xfs_dir2_isblock( if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK))) return rval; rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; - ASSERT(rval == 0 || args->dp->i_d.di_size == args->geo->blksize); + if (rval != 0 && args->dp->i_d.di_size != args->geo->blksize) + return -EFSCORRUPTED; *vp = rval; return 0; } diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index dd483e2..0091ac3 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -386,6 +386,7 @@ xfs_dinode_verify( xfs_ino_t ino, struct xfs_dinode *dip) { + uint16_t mode; uint16_t flags; uint64_t flags2; @@ -396,8 +397,10 @@ xfs_dinode_verify( if (be64_to_cpu(dip->di_size) & (1ULL << 63)) return false; - /* No zero-length symlinks. */ - if (S_ISLNK(be16_to_cpu(dip->di_mode)) && dip->di_size == 0) + mode = be16_to_cpu(dip->di_mode); + + /* No zero-length symlinks/dirs. */ + if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0) return false; /* only version 3 or greater inodes are extensively verified here */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/6] xfs: sanity check directory inode di_size 2017-01-11 8:24 ` [PATCH v8 2/6] xfs: sanity check directory inode di_size Amir Goldstein @ 2017-01-14 6:28 ` Darrick J. Wong 0 siblings, 0 replies; 14+ messages in thread From: Darrick J. Wong @ 2017-01-14 6:28 UTC (permalink / raw) To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 10:24:06AM +0200, Amir Goldstein wrote: > This changes fixes an assertion hit when fuzzing on-disk > i_mode values. > > The easy case to fix is when changing an empty file > i_mode to S_IFDIR. In this case, xfs_dinode_verify() > detects an illegal zero size for directory and fails > to load the inode structure from disk. > > For the case of non empty file whose i_mode is changed > to S_IFDIR, the ASSERT() statement in xfs_dir2_isblock() > is replaced with return -EFSCORRUPTED, to avoid interacting > with corrupted jusk also when XFS_DEBUG is disabled. > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/xfs/libxfs/xfs_dir2.c | 3 ++- > fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > index c58d72c..4f7913f 100644 > --- a/fs/xfs/libxfs/xfs_dir2.c > +++ b/fs/xfs/libxfs/xfs_dir2.c > @@ -631,7 +631,8 @@ xfs_dir2_isblock( > if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK))) > return rval; > rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; > - ASSERT(rval == 0 || args->dp->i_d.di_size == args->geo->blksize); > + if (rval != 0 && args->dp->i_d.di_size != args->geo->blksize) > + return -EFSCORRUPTED; > *vp = rval; > return 0; > } > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index dd483e2..0091ac3 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -386,6 +386,7 @@ xfs_dinode_verify( > xfs_ino_t ino, > struct xfs_dinode *dip) > { > + uint16_t mode; > uint16_t flags; > uint64_t flags2; > > @@ -396,8 +397,10 @@ xfs_dinode_verify( > if (be64_to_cpu(dip->di_size) & (1ULL << 63)) > return false; > > - /* No zero-length symlinks. */ > - if (S_ISLNK(be16_to_cpu(dip->di_mode)) && dip->di_size == 0) > + mode = be16_to_cpu(dip->di_mode); > + > + /* No zero-length symlinks/dirs. */ > + if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0) Drat, I just hit this with the fuzzer tests, so I'll pull this in for rc5. Good work! :) --D > return false; > > /* only version 3 or greater inodes are extensively verified here */ > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 1/6] xfs: make the ASSERT() condition likely Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 2/6] xfs: sanity check directory inode di_size Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-11 8:38 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein ` (2 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs xfs_dir2.h dereferences some data types in inline functions and fails to include those type definitions, e.g.: xfs_dir2_data_aoff_t, struct xfs_da_geometry. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/libxfs/xfs_dir2.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h index 0197590..72df0dc 100644 --- a/fs/xfs/libxfs/xfs_dir2.h +++ b/fs/xfs/libxfs/xfs_dir2.h @@ -18,6 +18,9 @@ #ifndef __XFS_DIR2_H__ #define __XFS_DIR2_H__ +#include "xfs_da_format.h" +#include "xfs_da_btree.h" + struct xfs_defer_ops; struct xfs_da_args; struct xfs_inode; -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h 2017-01-11 8:24 ` [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein @ 2017-01-11 8:38 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2017-01-11 8:38 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 10:24:07AM +0200, Amir Goldstein wrote: > xfs_dir2.h dereferences some data types in inline functions > and fails to include those type definitions, e.g.: > xfs_dir2_data_aoff_t, struct xfs_da_geometry. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> I'd normally fold this into the patch that actually exposes the dependency. But except for that it looks fine to me: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (2 preceding siblings ...) 2017-01-11 8:24 ` [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-11 8:38 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 6/6] xfs: sanity check inode di_mode Amir Goldstein 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs The size of the xfs_mode_to_ftype[] conversion table was too small to handle an invalid value of mode=S_IFMT. Instead of fixing the table size, replace the conversion table with a conversion helper that uses a switch statement. Suggested-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/libxfs/xfs_dir2.c | 36 ++++++++++++++++++++++-------------- fs/xfs/libxfs/xfs_dir2.h | 5 ++--- fs/xfs/xfs_iops.c | 2 +- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index 4f7913f..eb64f38 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -36,21 +36,29 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR }; /* - * @mode, if set, indicates that the type field needs to be set up. - * This uses the transformation from file mode to DT_* as defined in linux/fs.h - * for file type specification. This will be propagated into the directory - * structure if appropriate for the given operation and filesystem config. + * Convert inode mode to directory entry filetype */ -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = { - [0] = XFS_DIR3_FT_UNKNOWN, - [S_IFREG >> S_SHIFT] = XFS_DIR3_FT_REG_FILE, - [S_IFDIR >> S_SHIFT] = XFS_DIR3_FT_DIR, - [S_IFCHR >> S_SHIFT] = XFS_DIR3_FT_CHRDEV, - [S_IFBLK >> S_SHIFT] = XFS_DIR3_FT_BLKDEV, - [S_IFIFO >> S_SHIFT] = XFS_DIR3_FT_FIFO, - [S_IFSOCK >> S_SHIFT] = XFS_DIR3_FT_SOCK, - [S_IFLNK >> S_SHIFT] = XFS_DIR3_FT_SYMLINK, -}; +const unsigned char xfs_mode_to_ftype(int mode) +{ + switch (mode & S_IFMT) { + case S_IFREG: + return XFS_DIR3_FT_REG_FILE; + case S_IFDIR: + return XFS_DIR3_FT_DIR; + case S_IFCHR: + return XFS_DIR3_FT_CHRDEV; + case S_IFBLK: + return XFS_DIR3_FT_BLKDEV; + case S_IFIFO: + return XFS_DIR3_FT_FIFO; + case S_IFSOCK: + return XFS_DIR3_FT_SOCK; + case S_IFLNK: + return XFS_DIR3_FT_SYMLINK; + default: + return XFS_DIR3_FT_UNKNOWN; + } +} /* * ASCII case-insensitive (ie. A-Z) support for directories that was diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h index 72df0dc..d4b77ab 100644 --- a/fs/xfs/libxfs/xfs_dir2.h +++ b/fs/xfs/libxfs/xfs_dir2.h @@ -35,10 +35,9 @@ struct xfs_dir2_data_unused; extern struct xfs_name xfs_name_dotdot; /* - * directory filetype conversion tables. + * Convert inode mode to directory entry filetype */ -#define S_SHIFT 12 -extern const unsigned char xfs_mode_to_ftype[]; +extern const unsigned char xfs_mode_to_ftype(int mode); /* * directory operations vector for encode/decode routines diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 308bebb..821f08d 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -103,7 +103,7 @@ xfs_dentry_to_name( { namep->name = dentry->d_name.name; namep->len = dentry->d_name.len; - namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT]; + namep->type = xfs_mode_to_ftype(mode); } STATIC void -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement 2017-01-11 8:24 ` [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein @ 2017-01-11 8:38 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2017-01-11 8:38 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 10:24:08AM +0200, Amir Goldstein wrote: > The size of the xfs_mode_to_ftype[] conversion table > was too small to handle an invalid value of mode=S_IFMT. > > Instead of fixing the table size, replace the conversion table > with a conversion helper that uses a switch statement. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (3 preceding siblings ...) 2017-01-11 8:24 ` [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-11 8:42 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 6/6] xfs: sanity check inode di_mode Amir Goldstein 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs The helper xfs_dentry_to_name() is used by 2 different classes of callers: Callers that pass zero mode and don't care about the returned name.type field and Callers that pass non zero mode and do care about the name.type field. Change xfs_dentry_to_name() to not take the mode argument and change the call sites of the first class to not pass the mode argument. Create a new helper xfs_dentry_mode_to_name() which does pass the mode argument and returns -EFSCORRUPTED if mode is invalid. Callers that translate non zero mode to on-disk file type now check the return value and will export the error to user instead of staging an invalid file type to be written to directory entry. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/xfs_iops.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 821f08d..22c1615 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -98,12 +98,27 @@ xfs_init_security( static void xfs_dentry_to_name( struct xfs_name *namep, + struct dentry *dentry) +{ + namep->name = dentry->d_name.name; + namep->len = dentry->d_name.len; + namep->type = XFS_DIR3_FT_UNKNOWN; +} + +static int +xfs_dentry_mode_to_name( + struct xfs_name *namep, struct dentry *dentry, int mode) { namep->name = dentry->d_name.name; namep->len = dentry->d_name.len; namep->type = xfs_mode_to_ftype(mode); + + if (unlikely(namep->type == XFS_DIR3_FT_UNKNOWN)) + return -EFSCORRUPTED; + + return 0; } STATIC void @@ -119,7 +134,7 @@ xfs_cleanup_inode( * xfs_init_security we must back out. * ENOSPC can hit here, among other things. */ - xfs_dentry_to_name(&teardown, dentry, 0); + xfs_dentry_to_name(&teardown, dentry); xfs_remove(XFS_I(dir), &teardown, XFS_I(inode)); } @@ -154,8 +169,12 @@ xfs_generic_create( if (error) return error; + /* Verify mode is valid also for tmpfile case */ + error = xfs_dentry_mode_to_name(&name, dentry, mode); + if (unlikely(error)) + goto out_free_acl; + if (!tmpfile) { - xfs_dentry_to_name(&name, dentry, mode); error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip); } else { error = xfs_create_tmpfile(XFS_I(dir), dentry, mode, &ip); @@ -248,7 +267,7 @@ xfs_vn_lookup( if (dentry->d_name.len >= MAXNAMELEN) return ERR_PTR(-ENAMETOOLONG); - xfs_dentry_to_name(&name, dentry, 0); + xfs_dentry_to_name(&name, dentry); error = xfs_lookup(XFS_I(dir), &name, &cip, NULL); if (unlikely(error)) { if (unlikely(error != -ENOENT)) @@ -275,7 +294,7 @@ xfs_vn_ci_lookup( if (dentry->d_name.len >= MAXNAMELEN) return ERR_PTR(-ENAMETOOLONG); - xfs_dentry_to_name(&xname, dentry, 0); + xfs_dentry_to_name(&xname, dentry); error = xfs_lookup(XFS_I(dir), &xname, &ip, &ci_name); if (unlikely(error)) { if (unlikely(error != -ENOENT)) @@ -310,7 +329,9 @@ xfs_vn_link( struct xfs_name name; int error; - xfs_dentry_to_name(&name, dentry, inode->i_mode); + error = xfs_dentry_mode_to_name(&name, dentry, inode->i_mode); + if (unlikely(error)) + return error; error = xfs_link(XFS_I(dir), XFS_I(inode), &name); if (unlikely(error)) @@ -329,7 +350,7 @@ xfs_vn_unlink( struct xfs_name name; int error; - xfs_dentry_to_name(&name, dentry, 0); + xfs_dentry_to_name(&name, dentry); error = xfs_remove(XFS_I(dir), &name, XFS_I(d_inode(dentry))); if (error) @@ -359,7 +380,9 @@ xfs_vn_symlink( mode = S_IFLNK | (irix_symlink_mode ? 0777 & ~current_umask() : S_IRWXUGO); - xfs_dentry_to_name(&name, dentry, mode); + error = xfs_dentry_mode_to_name(&name, dentry, mode); + if (unlikely(error)) + goto out; error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip); if (unlikely(error)) @@ -395,6 +418,7 @@ xfs_vn_rename( { struct inode *new_inode = d_inode(ndentry); int omode = 0; + int error; struct xfs_name oname; struct xfs_name nname; @@ -405,8 +429,14 @@ xfs_vn_rename( if (flags & RENAME_EXCHANGE) omode = d_inode(ndentry)->i_mode; - xfs_dentry_to_name(&oname, odentry, omode); - xfs_dentry_to_name(&nname, ndentry, d_inode(odentry)->i_mode); + error = xfs_dentry_mode_to_name(&oname, odentry, omode); + if (omode && unlikely(error)) + return error; + + error = xfs_dentry_mode_to_name(&nname, ndentry, + d_inode(odentry)->i_mode); + if (unlikely(error)) + return error; return xfs_rename(XFS_I(odir), &oname, XFS_I(d_inode(odentry)), XFS_I(ndir), &nname, -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-11 8:24 ` [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein @ 2017-01-11 8:42 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2017-01-11 8:42 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Brian Foster, Christoph Hellwig, linux-xfs Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> Although I wonder if we should simply pass down qstr ( + mode if needed) and kill off struct xfs_name.. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 6/6] xfs: sanity check inode di_mode 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (4 preceding siblings ...) 2017-01-11 8:24 ` [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein @ 2017-01-11 8:24 ` Amir Goldstein 2017-01-11 8:42 ` Christoph Hellwig 5 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-01-11 8:24 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs Check for invalid file type in xfs_dinode_verify() and fail to load the inode structure from disk. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/libxfs/xfs_inode_buf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index 0091ac3..d93f9d9 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -29,6 +29,7 @@ #include "xfs_icache.h" #include "xfs_trans.h" #include "xfs_ialloc.h" +#include "xfs_dir2.h" /* * Check that none of the inode's in the buffer have a next @@ -398,6 +399,8 @@ xfs_dinode_verify( return false; mode = be16_to_cpu(dip->di_mode); + if (mode && xfs_mode_to_ftype(mode) == XFS_DIR3_FT_UNKNOWN) + return false; /* No zero-length symlinks/dirs. */ if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0) -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 6/6] xfs: sanity check inode di_mode 2017-01-11 8:24 ` [PATCH v8 6/6] xfs: sanity check inode di_mode Amir Goldstein @ 2017-01-11 8:42 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2017-01-11 8:42 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 10:24:10AM +0200, Amir Goldstein wrote: > Check for invalid file type in xfs_dinode_verify() > and fail to load the inode structure from disk. > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-01-18 20:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-11 8:24 [PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-11 8:24 ` [PATCH v8 1/6] xfs: make the ASSERT() condition likely Amir Goldstein 2017-01-13 7:25 ` Christoph Hellwig 2017-01-18 20:57 ` Darrick J. Wong 2017-01-11 8:24 ` [PATCH v8 2/6] xfs: sanity check directory inode di_size Amir Goldstein 2017-01-14 6:28 ` Darrick J. Wong 2017-01-11 8:24 ` [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein 2017-01-11 8:38 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein 2017-01-11 8:38 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein 2017-01-11 8:42 ` Christoph Hellwig 2017-01-11 8:24 ` [PATCH v8 6/6] xfs: sanity check inode di_mode Amir Goldstein 2017-01-11 8:42 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.