On Mon, Oct 23, 2017 at 04:49:03PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 23, 2017 at 10:46:43AM -0400, Brian Foster wrote:
> > If a malformatted filesystem is mounted and attempts log recovery,
> > we can end up passing garbage parameter values to
> > xlog_find_verify_log_record(). In turn, the latter can pass a NULL
> > head pointer to xlog_header_check_mount() and cause a kernel panic.
>
> Malformed how? Is *last_blk some huge value such that i < -1?
>
> I'm trying to figure out how we get passed a NULL head, and (afaict)
> that's one way it can happen...
>
Malformatted simply means the log is too small. What happens is that
start_blk underflows in xlog_find_head() due to:
start_blk = log_bbnum - (num_scan_bblks - head_blk);
... and the code ends up with a negative head_blk value by the time we
get to the "validate_head" label. last_blk ends up negative in
xlog_find_verify_log_record() and passes the NULL head pointer to
xlog_header_check_mount().
I suppose this might be a bit more obvious if we similarly fixed up
xlog_find_verify_cycle() to ensure that start_blk is sane, rather than
let it fall through to the record validation before failing.
> > Add some parameter sanity checks to both functions. Checks in both
> > places are technically not necessary, but do so to help future proof
> > the code. This prevents a kernel panic and replaces it with a more
> > graceful mount failure.
> >
> > Reported-by: Zorro Lang <zlang@xxxxxxxxxx>
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_log_recover.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index ee34899..80b37a2 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -347,9 +347,12 @@ xlog_header_check_recover(
> > */
> > STATIC int
> > xlog_header_check_mount(
> > - xfs_mount_t *mp,
> > - xlog_rec_header_t *head)
> > + struct xfs_mount *mp,
> > + struct xlog_rec_header *head)
> > {
> > + if (!head)
> > + return -EINVAL;
> > +
> > ASSERT(head->h_magicno == cpu_to_be32(XLOG_HEADER_MAGIC_NUM));
> >
> > if (uuid_is_null(&head->h_fs_uuid)) {
> > @@ -533,6 +536,10 @@ xlog_find_verify_log_record(
> >
> > ASSERT(start_blk != 0 || *last_blk != start_blk);
> >
> > + if (start_blk < 0 || start_blk > log->l_logBBsize ||
> > + *last_blk < 0 || *last_blk > log->l_logBBsize)
> > + return -EINVAL;
>
> /me stumbled over the fact that start_blk and last_blk are offsets (in
> units of basic blocks) within the log, not absolute disk offsets like
> their xfs_daddr_t type implies. :(
>
> Could you add a comment somewhere in this function explaining that these
> two "block" numbers are actually relative logBBstart? The comment
> implies this, but apparently not strongly enough.
>
Sure. I'll add a similar check to the cycle verifier as noted above and
add a comment in both places to note that we're looking for sane "log
relative block numbers."
Actually... now that I take a closer look at the code, I'm wondering if
a more robust solution than these explicit checks would be to push this
validation down to the log buffer helpers. We already have
xlog_buf_bbcount_valid() for checking the buffer length. Perhaps we
should enhance that to a 'xlog_buf_valid()' for sanity checking both the
log block address and count (and just passing 0 from xlog_get_bp())
before the blkno converted to a real daddr and actually read. That may
better protect us from going off the rails anywhere else in the future
since the read would simply fail. Thoughts?
Brian
> --D
>
> > +
> > if (!(bp = xlog_get_bp(log, num_blks))) {
> > if (!(bp = xlog_get_bp(log, 1)))
> > return -ENOMEM;
> > --
> > 2.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
