* [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
* [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 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 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
* 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
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.