* [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode @ 2017-01-10 15:39 Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein ` (5 more replies) 0 siblings, 6 replies; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw) To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs Darrick, This is 7th revision of the fixes for malformed on-disk i_mode. Patches 1-3 are simple cleanups, some already reviewed and one (patch 2) was suggested by you. Patch 4 replaces xfs_mode_to_ftype table with switch statement per Christoph's suggestion. Patches 5-6 use the new conversion helper to sanity test mode loaded from disk and export the error to the user. Tested with generic/401 with -n ftype=0|1. Tested with new xfs/348 test with -n ftype=0|1. Amir. 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 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 | 40 ++++++++++++++++++++++------------- fs/xfs/libxfs/xfs_dir2.h | 8 ++++--- fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++++-- fs/xfs/xfs_iops.c | 49 ++++++++++++++++++++++++++++++++++--------- fs/xfs/xfs_linux.h | 6 +++--- 5 files changed, 80 insertions(+), 33 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v7 1/6] xfs: make the ASSERT() condition likely 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein ` (4 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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] 12+ messages in thread
* [PATCH v7 2/6] xfs: sanity check directory inode di_size 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein ` (3 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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] 12+ messages in thread
* [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein ` (2 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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] 12+ messages in thread
* [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (2 preceding siblings ...) 2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 23:17 ` Darrick J. Wong 2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein 5 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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> 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] 12+ messages in thread
* Re: [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement 2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein @ 2017-01-10 23:17 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2017-01-10 23:17 UTC (permalink / raw) To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Tue, Jan 10, 2017 at 05:39:15PM +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> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > 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 > > -- > 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] 12+ messages in thread
* [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (3 preceding siblings ...) 2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 23:15 ` Darrick J. Wong 2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein 5 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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 | 47 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 821f08d..ef38d0f 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,8 @@ 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); + ASSERT(error == 0); error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip); if (unlikely(error)) @@ -395,6 +417,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 +428,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] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein @ 2017-01-10 23:15 ` Darrick J. Wong 2017-01-11 5:08 ` Amir Goldstein 0 siblings, 1 reply; 12+ messages in thread From: Darrick J. Wong @ 2017-01-10 23:15 UTC (permalink / raw) To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote: > 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 | 47 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 38 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index 821f08d..ef38d0f 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,8 @@ 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); > + ASSERT(error == 0); Why not the usual 'if (error) goto out;' here? --D > > error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip); > if (unlikely(error)) > @@ -395,6 +417,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 +428,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 > > -- > 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] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-10 23:15 ` Darrick J. Wong @ 2017-01-11 5:08 ` Amir Goldstein 2017-01-11 7:50 ` Darrick J. Wong 0 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-01-11 5:08 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 1:15 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote: >> 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> >> --- ... >> >> 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); >> + ASSERT(error == 0); > > Why not the usual 'if (error) goto out;' here? > Because mode here is not coming from disk, nor from user input, so an error from converting S_IFLNK would be a programming bug. We could also ignore the return value in this case, let me know if you prefer that. BTW, process question. Do you need me to re-send patched with your Reviewed-by, or will you add those when applying? Thanks, Amir. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry 2017-01-11 5:08 ` Amir Goldstein @ 2017-01-11 7:50 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2017-01-11 7:50 UTC (permalink / raw) To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Wed, Jan 11, 2017 at 07:08:57AM +0200, Amir Goldstein wrote: > On Wed, Jan 11, 2017 at 1:15 AM, Darrick J. Wong > <darrick.wong@oracle.com> wrote: > > On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote: > >> 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> > >> --- > ... > >> > >> 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); > >> + ASSERT(error == 0); > > > > Why not the usual 'if (error) goto out;' here? > > > > Because mode here is not coming from disk, nor from user input, > so an error from converting S_IFLNK would be a programming bug. > We could also ignore the return value in this case, let me know if you > prefer that. Nah, it's just eyebrow-raising since it's a deviation from the usual paradigm. > BTW, process question. Do you need me to re-send patched with > your Reviewed-by, or will you add those when applying? In general I plan to pull the most recent patch mail and add whatever tags come in as replies, so if you resend a patch please add the tags yourself. --D > > Thanks, > Amir. > -- > 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] 12+ messages in thread
* [PATCH v7 6/6] xfs: sanity check inode di_mode 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein ` (4 preceding siblings ...) 2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein @ 2017-01-10 15:39 ` Amir Goldstein 2017-01-10 23:16 ` Darrick J. Wong 5 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-01-10 15:39 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. 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] 12+ messages in thread
* Re: [PATCH v7 6/6] xfs: sanity check inode di_mode 2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein @ 2017-01-10 23:16 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2017-01-10 23:16 UTC (permalink / raw) To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs On Tue, Jan 10, 2017 at 05:39:17PM +0200, Amir Goldstein wrote: > Check for invalid file type in xfs_dinode_verify() > and fail to load the inode structure from disk. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > 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 > > -- > 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] 12+ messages in thread
end of thread, other threads:[~2017-01-11 7:50 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein 2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein 2017-01-10 23:17 ` Darrick J. Wong 2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein 2017-01-10 23:15 ` Darrick J. Wong 2017-01-11 5:08 ` Amir Goldstein 2017-01-11 7:50 ` Darrick J. Wong 2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein 2017-01-10 23:16 ` Darrick J. Wong
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.