* [PATCH 0/2] xfs: validate size vs format, take 2 @ 2018-09-10 22:18 Eric Sandeen 2018-09-10 22:22 ` [PATCH 1/2] xfs: validate inode di_forkoff Eric Sandeen 2018-09-10 22:24 ` [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs Eric Sandeen 0 siblings, 2 replies; 6+ messages in thread From: Eric Sandeen @ 2018-09-10 22:18 UTC (permalink / raw) To: linux-xfs [PATCH] xfs: verify size-vs-format for symlinks & dirs redux, now with more cowbell^Wdi_forkoff validation. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] xfs: validate inode di_forkoff 2018-09-10 22:18 [PATCH 0/2] xfs: validate size vs format, take 2 Eric Sandeen @ 2018-09-10 22:22 ` Eric Sandeen 2018-09-24 17:04 ` Brian Foster 2018-09-10 22:24 ` [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs Eric Sandeen 1 sibling, 1 reply; 6+ messages in thread From: Eric Sandeen @ 2018-09-10 22:22 UTC (permalink / raw) To: Eric Sandeen, linux-xfs Verify the inode di_forkoff, lifted from xfs_repair's process_check_inode_forkoff(). Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index 30d1d60f1d46..8d76637a49a7 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -415,6 +415,31 @@ xfs_dinode_verify_fork( return NULL; } +static xfs_failaddr_t +xfs_dinode_verify_forkoff( + struct xfs_dinode *dip, + struct xfs_mount *mp) +{ + if (dip->di_forkoff == 0) + return NULL; + + switch (dip->di_format) { + case XFS_DINODE_FMT_DEV: + if (dip->di_forkoff != (roundup(sizeof(xfs_dev_t), 8) >> 3)) + return __this_address; + break; + case XFS_DINODE_FMT_LOCAL: /* fall through ... */ + case XFS_DINODE_FMT_EXTENTS: /* fall through ... */ + case XFS_DINODE_FMT_BTREE: + if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3)) + return __this_address; + break; + default: + return __this_address; + } + return NULL; +} + xfs_failaddr_t xfs_dinode_verify( struct xfs_mount *mp, @@ -470,6 +498,11 @@ xfs_dinode_verify( if (mode && (flags & XFS_DIFLAG_REALTIME) && !mp->m_rtdev_targp) return __this_address; + /* check for illegal values of di_forkoff */ + fa = xfs_dinode_verify_forkoff(dip, mp); + if (fa) + return fa; + /* Do we have appropriate data fork formats for the mode? */ switch (mode & S_IFMT) { case S_IFIFO: ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] xfs: validate inode di_forkoff 2018-09-10 22:22 ` [PATCH 1/2] xfs: validate inode di_forkoff Eric Sandeen @ 2018-09-24 17:04 ` Brian Foster 2018-09-25 2:50 ` Eric Sandeen 0 siblings, 1 reply; 6+ messages in thread From: Brian Foster @ 2018-09-24 17:04 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Mon, Sep 10, 2018 at 05:22:08PM -0500, Eric Sandeen wrote: > Verify the inode di_forkoff, lifted from xfs_repair's > process_check_inode_forkoff(). > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index 30d1d60f1d46..8d76637a49a7 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -415,6 +415,31 @@ xfs_dinode_verify_fork( > return NULL; > } > > +static xfs_failaddr_t > +xfs_dinode_verify_forkoff( > + struct xfs_dinode *dip, > + struct xfs_mount *mp) > +{ > + if (dip->di_forkoff == 0) > + return NULL; I think it would be good to use XFS_DFORK_Q() here, just to be consistent with the other, similar checks. Otherwise looks good: Reviewed-by: Brian Foster <bfoster@redhat.com> > + > + switch (dip->di_format) { > + case XFS_DINODE_FMT_DEV: > + if (dip->di_forkoff != (roundup(sizeof(xfs_dev_t), 8) >> 3)) > + return __this_address; > + break; > + case XFS_DINODE_FMT_LOCAL: /* fall through ... */ > + case XFS_DINODE_FMT_EXTENTS: /* fall through ... */ > + case XFS_DINODE_FMT_BTREE: > + if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3)) > + return __this_address; > + break; > + default: > + return __this_address; > + } > + return NULL; > +} > + > xfs_failaddr_t > xfs_dinode_verify( > struct xfs_mount *mp, > @@ -470,6 +498,11 @@ xfs_dinode_verify( > if (mode && (flags & XFS_DIFLAG_REALTIME) && !mp->m_rtdev_targp) > return __this_address; > > + /* check for illegal values of di_forkoff */ > + fa = xfs_dinode_verify_forkoff(dip, mp); > + if (fa) > + return fa; > + > /* Do we have appropriate data fork formats for the mode? */ > switch (mode & S_IFMT) { > case S_IFIFO: > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] xfs: validate inode di_forkoff 2018-09-24 17:04 ` Brian Foster @ 2018-09-25 2:50 ` Eric Sandeen 0 siblings, 0 replies; 6+ messages in thread From: Eric Sandeen @ 2018-09-25 2:50 UTC (permalink / raw) To: Brian Foster, Eric Sandeen; +Cc: linux-xfs On 9/24/18 12:04 PM, Brian Foster wrote: > On Mon, Sep 10, 2018 at 05:22:08PM -0500, Eric Sandeen wrote: >> Verify the inode di_forkoff, lifted from xfs_repair's >> process_check_inode_forkoff(). >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c >> index 30d1d60f1d46..8d76637a49a7 100644 >> --- a/fs/xfs/libxfs/xfs_inode_buf.c >> +++ b/fs/xfs/libxfs/xfs_inode_buf.c >> @@ -415,6 +415,31 @@ xfs_dinode_verify_fork( >> return NULL; >> } >> >> +static xfs_failaddr_t >> +xfs_dinode_verify_forkoff( >> + struct xfs_dinode *dip, >> + struct xfs_mount *mp) >> +{ >> + if (dip->di_forkoff == 0) >> + return NULL; > > I think it would be good to use XFS_DFORK_Q() here, just to be > consistent with the other, similar checks. Otherwise looks good: Ok; personally I hate that macro ;) but you're right, consistency first. -Eric ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs 2018-09-10 22:18 [PATCH 0/2] xfs: validate size vs format, take 2 Eric Sandeen 2018-09-10 22:22 ` [PATCH 1/2] xfs: validate inode di_forkoff Eric Sandeen @ 2018-09-10 22:24 ` Eric Sandeen 2018-09-24 17:06 ` Brian Foster 1 sibling, 1 reply; 6+ messages in thread From: Eric Sandeen @ 2018-09-10 22:24 UTC (permalink / raw) To: Eric Sandeen, linux-xfs Today, xfs_ifork_verify_data() will simply skip verification if the inode claims to be in non-local format. However, nothing catches the case where the size for the format is too small to be non-local. xfs_repair tests for this mismatch in process_check_inode_sizes(), so do the same in this verifier. Reported-by: Xu, Wen <wen.xu@gatech.edu> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: restructure code & tests per Dave's suggestion on the V1 patch. diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 183ec0cb8921..d6a137f5e207 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -732,12 +732,32 @@ xfs_ifork_verify_data( struct xfs_inode *ip, struct xfs_ifork_ops *ops) { - /* Non-local data fork, we're done. */ - if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) + struct xfs_mount *mp = ip->i_mount; + int mode = VFS_I(ip)->i_mode; + + if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) { + /* + * types that can be in local form need size checks + * to ensure they have the right amount of data in + * them to be in non-local form + */ + switch (mode & S_IFMT) { + case S_IFDIR: + if (ip->i_d.di_size < mp->m_dir_geo->blksize) + return __this_address; + break; + case S_IFLNK: + if (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip)) + return __this_address; + break; + default: + break; + } return NULL; + } /* Check the inline data fork if there is one. */ - switch (VFS_I(ip)->i_mode & S_IFMT) { + switch (mode & S_IFMT) { case S_IFDIR: return ops->verify_dir(ip); case S_IFLNK: ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs 2018-09-10 22:24 ` [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs Eric Sandeen @ 2018-09-24 17:06 ` Brian Foster 0 siblings, 0 replies; 6+ messages in thread From: Brian Foster @ 2018-09-24 17:06 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Mon, Sep 10, 2018 at 05:24:17PM -0500, Eric Sandeen wrote: > Today, xfs_ifork_verify_data() will simply skip verification if the inode > claims to be in non-local format. However, nothing catches the case where > the size for the format is too small to be non-local. xfs_repair tests > for this mismatch in process_check_inode_sizes(), so do the same in this > verifier. > > Reported-by: Xu, Wen <wen.xu@gatech.edu> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > V2: restructure code & tests per Dave's suggestion on the V1 patch. > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > index 183ec0cb8921..d6a137f5e207 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.c > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -732,12 +732,32 @@ xfs_ifork_verify_data( > struct xfs_inode *ip, > struct xfs_ifork_ops *ops) > { > - /* Non-local data fork, we're done. */ > - if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) > + struct xfs_mount *mp = ip->i_mount; > + int mode = VFS_I(ip)->i_mode; > + > + if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) { > + /* > + * types that can be in local form need size checks > + * to ensure they have the right amount of data in > + * them to be in non-local form > + */ Just another nit... I had to read the comment a few times to understand how it relates to the code. Could we perhaps move it above the if check and reword it to something like: /* * If this is a non-local format directory or symlink, verify the inode * size is consistent with non-local format. */ > + switch (mode & S_IFMT) { > + case S_IFDIR: > + if (ip->i_d.di_size < mp->m_dir_geo->blksize) > + return __this_address; Hmm, where does the ->blksize check come from? The dir shortform transition code looks like it keys off of XFS_IFORK_DSIZE().. Ah Ok, I went back and read Dave's feedback on the v1 post. If we want to use this to also cover a range of invalid dir sizes, could we incorporate that into the above comment as well so it's more clear? E.g., change the above to something like: /* * Verify non-local format forks have a valid size. Symlinks must have * outgrown the data fork size. The same goes for non-local dirs, but * dirs grow at dirblock granularity. Perform a slightly stronger check * and require the dir is at least one dirblock in size. */ Otherwise with the comment fixups: Reviewed-by: Brian Foster <bfoster@redhat.com> > + break; > + case S_IFLNK: > + if (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip)) > + return __this_address; > + break; > + default: > + break; > + } > return NULL; > + } > > /* Check the inline data fork if there is one. */ > - switch (VFS_I(ip)->i_mode & S_IFMT) { > + switch (mode & S_IFMT) { > case S_IFDIR: > return ops->verify_dir(ip); > case S_IFLNK: > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-25 8:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-10 22:18 [PATCH 0/2] xfs: validate size vs format, take 2 Eric Sandeen 2018-09-10 22:22 ` [PATCH 1/2] xfs: validate inode di_forkoff Eric Sandeen 2018-09-24 17:04 ` Brian Foster 2018-09-25 2:50 ` Eric Sandeen 2018-09-10 22:24 ` [PATCH 2/2 V2] xfs: verify size-vs-format for symlinks & dirs Eric Sandeen 2018-09-24 17:06 ` Brian Foster
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.