| Message ID | 20180619024128.22669-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jun 19, 2018 at 12:41:28PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > When the inode is in extent format, it can't have more extents that > fit in the inode fork. We don't currenty check this, and so this > corruption goes unnoticed by the inode verifiers. This can lead to > crashes operating on invalid in-memory structures. > > Attempts to access such a inode will now error out in the verifier > rather than allowing modification operations to proceed. > > Reported-by: Wen Xu <wen.xu@gatech.edu> > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_format.h | 3 ++ > fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- > 2 files changed, 48 insertions(+), 29 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 1c5a8aaf2bfc..1cb298fec274 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { > XFS_DFORK_DSIZE(dip, mp) : \ > XFS_DFORK_ASIZE(dip, mp)) > > +#define XFS_DFORK_MAXEXT(dip, mp, w) \ > + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) > + > /* > * Return pointers to the data or attribute forks. > */ > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index d38d724534c4..a41b6e5519e0 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( > } > } > > +static xfs_failaddr_t > +xfs_dinode_verify_fork( > + struct xfs_dinode *dip, > + struct xfs_mount *mp, > + int whichfork) > +{ > + uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); > + > + switch (XFS_DFORK_FORMAT(dip, whichfork)) { > + case XFS_DINODE_FMT_LOCAL: > + /* > + * no local regular files yet > + */ > + if (whichfork == XFS_DATA_FORK) { > + if (S_ISREG(be16_to_cpu(dip->di_mode))) > + return __this_address; > + if (be64_to_cpu(dip->di_size) > > + XFS_DFORK_SIZE(dip, mp, whichfork)) > + return __this_address; > + } > + if (di_nextents) > + return __this_address; > + /* fall through */ We could break here too, right? There's no point in further checks of di_nextents for local format forks. > + case XFS_DINODE_FMT_EXTENTS: > + if (di_nextents > XFS_DFORK_MAXEXT(dip, mp, whichfork)) > + return __this_address; Are we supposed to break here? --D > + case XFS_DINODE_FMT_BTREE: > + if (whichfork == XFS_ATTR_FORK) > + if (di_nextents > MAXAEXTNUM) > + return __this_address; > + else if (di_nextents > MAXEXTNUM) > + return __this_address; > + break; > + default: > + return __this_address; > + } > + return NULL; > +} > + > xfs_failaddr_t > xfs_dinode_verify( > struct xfs_mount *mp, > @@ -441,24 +480,9 @@ xfs_dinode_verify( > case S_IFREG: > case S_IFLNK: > case S_IFDIR: > - switch (dip->di_format) { > - case XFS_DINODE_FMT_LOCAL: > - /* > - * no local regular files yet > - */ > - if (S_ISREG(mode)) > - return __this_address; > - if (di_size > XFS_DFORK_DSIZE(dip, mp)) > - return __this_address; > - if (dip->di_nextents) > - return __this_address; > - /* fall through */ > - case XFS_DINODE_FMT_EXTENTS: > - case XFS_DINODE_FMT_BTREE: > - break; > - default: > - return __this_address; > - } > + fa = xfs_dinode_verify_fork(dip, mp, XFS_DATA_FORK); > + if (fa) > + return fa; > break; > case 0: > /* Uninitialized inode ok. */ > @@ -468,17 +492,9 @@ xfs_dinode_verify( > } > > if (XFS_DFORK_Q(dip)) { > - switch (dip->di_aformat) { > - case XFS_DINODE_FMT_LOCAL: > - if (dip->di_anextents) > - return __this_address; > - /* fall through */ > - case XFS_DINODE_FMT_EXTENTS: > - case XFS_DINODE_FMT_BTREE: > - break; > - default: > - return __this_address; > - } > + fa = xfs_dinode_verify_fork(dip, mp, XFS_ATTR_FORK); > + if (fa) > + return fa; > } else { > /* > * If there is no fork offset, this may be a freshly-made inode > -- > 2.17.0 > > -- > 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 -- 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
On Mon, Jun 18, 2018 at 09:57:25PM -0700, Darrick J. Wong wrote: > On Tue, Jun 19, 2018 at 12:41:28PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > When the inode is in extent format, it can't have more extents that > > fit in the inode fork. We don't currenty check this, and so this > > corruption goes unnoticed by the inode verifiers. This can lead to > > crashes operating on invalid in-memory structures. > > > > Attempts to access such a inode will now error out in the verifier > > rather than allowing modification operations to proceed. > > > > Reported-by: Wen Xu <wen.xu@gatech.edu> > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/libxfs/xfs_format.h | 3 ++ > > fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- > > 2 files changed, 48 insertions(+), 29 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > index 1c5a8aaf2bfc..1cb298fec274 100644 > > --- a/fs/xfs/libxfs/xfs_format.h > > +++ b/fs/xfs/libxfs/xfs_format.h > > @@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { > > XFS_DFORK_DSIZE(dip, mp) : \ > > XFS_DFORK_ASIZE(dip, mp)) > > > > +#define XFS_DFORK_MAXEXT(dip, mp, w) \ > > + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) > > + > > /* > > * Return pointers to the data or attribute forks. > > */ > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > > index d38d724534c4..a41b6e5519e0 100644 > > --- a/fs/xfs/libxfs/xfs_inode_buf.c > > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > > @@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( > > } > > } > > > > +static xfs_failaddr_t > > +xfs_dinode_verify_fork( > > + struct xfs_dinode *dip, > > + struct xfs_mount *mp, > > + int whichfork) > > +{ > > + uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); > > + > > + switch (XFS_DFORK_FORMAT(dip, whichfork)) { > > + case XFS_DINODE_FMT_LOCAL: > > + /* > > + * no local regular files yet > > + */ > > + if (whichfork == XFS_DATA_FORK) { > > + if (S_ISREG(be16_to_cpu(dip->di_mode))) > > + return __this_address; > > + if (be64_to_cpu(dip->di_size) > > > + XFS_DFORK_SIZE(dip, mp, whichfork)) > > + return __this_address; > > + } > > + if (di_nextents) > > + return __this_address; > > + /* fall through */ > > We could break here too, right? There's no point in further checks of > di_nextents for local format forks. > > > + case XFS_DINODE_FMT_EXTENTS: > > + if (di_nextents > XFS_DFORK_MAXEXT(dip, mp, whichfork)) > > + return __this_address; > > Are we supposed to break here? They all fall through like they used to, but they could break, too. The behaviour will be the same now. Cheers, Dave.
On Tue, Jun 19, 2018 at 03:29:31PM +1000, Dave Chinner wrote: > On Mon, Jun 18, 2018 at 09:57:25PM -0700, Darrick J. Wong wrote: > > On Tue, Jun 19, 2018 at 12:41:28PM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > When the inode is in extent format, it can't have more extents that > > > fit in the inode fork. We don't currenty check this, and so this > > > corruption goes unnoticed by the inode verifiers. This can lead to > > > crashes operating on invalid in-memory structures. > > > > > > Attempts to access such a inode will now error out in the verifier > > > rather than allowing modification operations to proceed. > > > > > > Reported-by: Wen Xu <wen.xu@gatech.edu> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > --- > > > fs/xfs/libxfs/xfs_format.h | 3 ++ > > > fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- > > > 2 files changed, 48 insertions(+), 29 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > > index 1c5a8aaf2bfc..1cb298fec274 100644 > > > --- a/fs/xfs/libxfs/xfs_format.h > > > +++ b/fs/xfs/libxfs/xfs_format.h > > > @@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { > > > XFS_DFORK_DSIZE(dip, mp) : \ > > > XFS_DFORK_ASIZE(dip, mp)) > > > > > > +#define XFS_DFORK_MAXEXT(dip, mp, w) \ > > > + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) > > > + > > > /* > > > * Return pointers to the data or attribute forks. > > > */ > > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > > > index d38d724534c4..a41b6e5519e0 100644 > > > --- a/fs/xfs/libxfs/xfs_inode_buf.c > > > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > > > @@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( > > > } > > > } > > > > > > +static xfs_failaddr_t > > > +xfs_dinode_verify_fork( > > > + struct xfs_dinode *dip, > > > + struct xfs_mount *mp, > > > + int whichfork) > > > +{ > > > + uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + > > > + switch (XFS_DFORK_FORMAT(dip, whichfork)) { > > > + case XFS_DINODE_FMT_LOCAL: > > > + /* > > > + * no local regular files yet > > > + */ > > > + if (whichfork == XFS_DATA_FORK) { > > > + if (S_ISREG(be16_to_cpu(dip->di_mode))) > > > + return __this_address; > > > + if (be64_to_cpu(dip->di_size) > > > > + XFS_DFORK_SIZE(dip, mp, whichfork)) > > > + return __this_address; > > > + } > > > + if (di_nextents) > > > + return __this_address; > > > + /* fall through */ > > > > We could break here too, right? There's no point in further checks of > > di_nextents for local format forks. > > > > > + case XFS_DINODE_FMT_EXTENTS: > > > + if (di_nextents > XFS_DFORK_MAXEXT(dip, mp, whichfork)) > > > + return __this_address; > > > > Are we supposed to break here? > > They all fall through like they used to, but they could break, too. > The behaviour will be the same now. Fair enough. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > 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 -- 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
On Tue, Jun 19, 2018 at 12:41:28PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > When the inode is in extent format, it can't have more extents that > fit in the inode fork. We don't currenty check this, and so this > corruption goes unnoticed by the inode verifiers. This can lead to > crashes operating on invalid in-memory structures. > > Attempts to access such a inode will now error out in the verifier > rather than allowing modification operations to proceed. > > Reported-by: Wen Xu <wen.xu@gatech.edu> > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_format.h | 3 ++ > fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- > 2 files changed, 48 insertions(+), 29 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 1c5a8aaf2bfc..1cb298fec274 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { > XFS_DFORK_DSIZE(dip, mp) : \ > XFS_DFORK_ASIZE(dip, mp)) > > +#define XFS_DFORK_MAXEXT(dip, mp, w) \ > + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) struct xfs_bmbt_rec, please. Also do we really need this macro instead of just open coding it? > + if (di_nextents) > + return __this_address; > + /* fall through */ seems weird to fall through when the next check is just for di_nextents again. I'd rather break out of the switch and have the common validation after it. But the basic of the patch look fine to me. -- 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
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 1c5a8aaf2bfc..1cb298fec274 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT(dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) + /* * Return pointers to the data or attribute forks. */ diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index d38d724534c4..a41b6e5519e0 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( } } +static xfs_failaddr_t +xfs_dinode_verify_fork( + struct xfs_dinode *dip, + struct xfs_mount *mp, + int whichfork) +{ + uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); + + switch (XFS_DFORK_FORMAT(dip, whichfork)) { + case XFS_DINODE_FMT_LOCAL: + /* + * no local regular files yet + */ + if (whichfork == XFS_DATA_FORK) { + if (S_ISREG(be16_to_cpu(dip->di_mode))) + return __this_address; + if (be64_to_cpu(dip->di_size) > + XFS_DFORK_SIZE(dip, mp, whichfork)) + return __this_address; + } + if (di_nextents) + return __this_address; + /* fall through */ + case XFS_DINODE_FMT_EXTENTS: + if (di_nextents > XFS_DFORK_MAXEXT(dip, mp, whichfork)) + return __this_address; + case XFS_DINODE_FMT_BTREE: + if (whichfork == XFS_ATTR_FORK) + if (di_nextents > MAXAEXTNUM) + return __this_address; + else if (di_nextents > MAXEXTNUM) + return __this_address; + break; + default: + return __this_address; + } + return NULL; +} + xfs_failaddr_t xfs_dinode_verify( struct xfs_mount *mp, @@ -441,24 +480,9 @@ xfs_dinode_verify( case S_IFREG: case S_IFLNK: case S_IFDIR: - switch (dip->di_format) { - case XFS_DINODE_FMT_LOCAL: - /* - * no local regular files yet - */ - if (S_ISREG(mode)) - return __this_address; - if (di_size > XFS_DFORK_DSIZE(dip, mp)) - return __this_address; - if (dip->di_nextents) - return __this_address; - /* fall through */ - case XFS_DINODE_FMT_EXTENTS: - case XFS_DINODE_FMT_BTREE: - break; - default: - return __this_address; - } + fa = xfs_dinode_verify_fork(dip, mp, XFS_DATA_FORK); + if (fa) + return fa; break; case 0: /* Uninitialized inode ok. */ @@ -468,17 +492,9 @@ xfs_dinode_verify( } if (XFS_DFORK_Q(dip)) { - switch (dip->di_aformat) { - case XFS_DINODE_FMT_LOCAL: - if (dip->di_anextents) - return __this_address; - /* fall through */ - case XFS_DINODE_FMT_EXTENTS: - case XFS_DINODE_FMT_BTREE: - break; - default: - return __this_address; - } + fa = xfs_dinode_verify_fork(dip, mp, XFS_ATTR_FORK); + if (fa) + return fa; } else { /* * If there is no fork offset, this may be a freshly-made inode