[1/4] xfs: sanity check log record range parameters
diff mbox

Message ID 20171023144646.50107-2-bfoster@redhat.com
State New
Headers show

Commit Message

Brian Foster Oct. 23, 2017, 2:46 p.m. UTC
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.

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@redhat.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong Oct. 23, 2017, 11:49 p.m. UTC | #1
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...

> 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@redhat.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  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.

--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@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
Brian Foster Oct. 24, 2017, 11:30 a.m. UTC | #2
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@redhat.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  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@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
Darrick J. Wong Oct. 25, 2017, 5:09 a.m. UTC | #3
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:
> > 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.

Agreed.

> > > 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@redhat.com>
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  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
> 
> > --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@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
--
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

Patch
diff mbox

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;
+
 	if (!(bp = xlog_get_bp(log, num_blks))) {
 		if (!(bp = xlog_get_bp(log, 1)))
 			return -ENOMEM;