On Tue, Oct 24, 2017 at 07:30:46AM -0400, Brian Foster wrote:

On Mon, Oct 23, 2017 at 04:49:03PM -0700, Darrick J. Wong wrote:

quoted

On Mon, Oct 23, 2017 at 10:46:43AM -0400, Brian Foster wrote:

quoted

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.

Agreed.

quoted

quoted

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 <redacted>
Signed-off-by: Brian Foster <redacted>
---
 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?

Sounds like a good idea.

xfs_verify_logbno?  In keeping with the xfs_verify_{agbno,fsbno,agino,ino,
dir_ino} that are getting added in 4.15?

--D

Brian

quoted

--D

quoted

+
 	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@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

--
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