[PATCH 0/2] libxfs: minor cleanups of destructors

[PATCH 0/2] libxfs: minor cleanups of destructors

[Darrick J. Wong]

Hi all,

Here are a couple more cleanup patches to prevent use-after-free of a
libxfs mount structure, and to refactor the device closure code.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=libxfs-cleanup-destructors

[PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
it during unmount.  Zero its contents to prevent any use-after-free.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/init.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/libxfs/init.c b/libxfs/init.c
index d4804ead..197690df 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -904,6 +904,7 @@ libxfs_umount(
 	if (mp->m_logdev_targp != mp->m_ddev_targp)
 		kmem_free(mp->m_logdev_targp);
 	kmem_free(mp->m_ddev_targp);
+	memset(mp, 0, sizeof(struct xfs_mount));
 
 	return error;
 }

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Eric Sandeen]

On 2/24/20 4:11 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
> it during unmount.  Zero its contents to prevent any use-after-free.

seems fine but makes me wonder what prompted it.  Did we have a use
after free?

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/init.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index d4804ead..197690df 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -904,6 +904,7 @@ libxfs_umount(
>  	if (mp->m_logdev_targp != mp->m_ddev_targp)
>  		kmem_free(mp->m_logdev_targp);
>  	kmem_free(mp->m_ddev_targp);
> +	memset(mp, 0, sizeof(struct xfs_mount));
>  
>  	return error;
>  }
> 

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Darrick J. Wong]

On Mon, Feb 24, 2020 at 09:57:03PM -0800, Eric Sandeen wrote:
> On 2/24/20 4:11 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
> > it during unmount.  Zero its contents to prevent any use-after-free.
> 
> seems fine but makes me wonder what prompted it.  Did we have a use
> after free?

No, just Brian musing about the possibility of it, so I said I'd zero
it out to make a UAF more obvious.

> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks for the review.

--D

> 
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/init.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > 
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index d4804ead..197690df 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -904,6 +904,7 @@ libxfs_umount(
> >  	if (mp->m_logdev_targp != mp->m_ddev_targp)
> >  		kmem_free(mp->m_logdev_targp);
> >  	kmem_free(mp->m_ddev_targp);
> > +	memset(mp, 0, sizeof(struct xfs_mount));
> >  
> >  	return error;
> >  }
> > 

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Brian Foster]

On Mon, Feb 24, 2020 at 04:11:20PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
> it during unmount.  Zero its contents to prevent any use-after-free.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  libxfs/init.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index d4804ead..197690df 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -904,6 +904,7 @@ libxfs_umount(
>  	if (mp->m_logdev_targp != mp->m_ddev_targp)
>  		kmem_free(mp->m_logdev_targp);
>  	kmem_free(mp->m_ddev_targp);
> +	memset(mp, 0, sizeof(struct xfs_mount));
>  
>  	return error;
>  }
> 

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Christoph Hellwig]

On Mon, Feb 24, 2020 at 04:11:20PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
> it during unmount.  Zero its contents to prevent any use-after-free.

I don't really this at all.  Seems to be cargo-cult style programming.

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Darrick J. Wong]

On Tue, Feb 25, 2020 at 09:40:38AM -0800, Christoph Hellwig wrote:
> On Mon, Feb 24, 2020 at 04:11:20PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
> > it during unmount.  Zero its contents to prevent any use-after-free.
> 
> I don't really this at all.  Seems to be cargo-cult style programming.

Admittedly I'm not convinced it's necessary either, seeing as we control
all the callers, and none of them actually screw this up.  But I defer
to the maintainer. ;)

(If anything I'm more afraid of the "libxfs_xinit_t x;" but that's a
different cleanup for another time.)

--D

Re: [PATCH 1/2] libxfs: zero the struct xfs_mount when unmounting the filesystem

[Eric Sandeen]

On 2/25/20 10:28 AM, Darrick J. Wong wrote:
> On Tue, Feb 25, 2020 at 09:40:38AM -0800, Christoph Hellwig wrote:
>> On Mon, Feb 24, 2020 at 04:11:20PM -0800, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Since libxfs doesn't allocate the struct xfs_mount *, we can't just free
>>> it during unmount.  Zero its contents to prevent any use-after-free.
>>
>> I don't really this at all.  Seems to be cargo-cult style programming.
> 
> Admittedly I'm not convinced it's necessary either, seeing as we control
> all the callers, and none of them actually screw this up.  But I defer
> to the maintainer. ;)

It struck me as odd as well, so despite my review I'll probably drop it,
given that there's no clear reason given to do this.

-Eric

[PATCH 2/2] libxfs: clean up libxfs_destroy

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

It's weird that libxfs_init opens the three devices passed in via the
libxfs_xinit structure but libxfs_destroy doesn't actually close them.
Fix this inconsistency and remove all the open-coded device closing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 copy/xfs_copy.c     |    2 +-
 db/init.c           |    8 +-------
 include/libxfs.h    |    2 +-
 libxfs/init.c       |   31 +++++++++++++++++++++++--------
 mkfs/xfs_mkfs.c     |    7 +------
 repair/xfs_repair.c |    7 +------
 6 files changed, 28 insertions(+), 29 deletions(-)


diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index a6d67038..7f4615ac 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -1200,7 +1200,7 @@ main(int argc, char **argv)
 
 	check_errors();
 	libxfs_umount(mp);
-	libxfs_destroy();
+	libxfs_destroy(&xargs);
 
 	return 0;
 }
diff --git a/db/init.c b/db/init.c
index 0ac37368..e5450d2b 100644
--- a/db/init.c
+++ b/db/init.c
@@ -217,13 +217,7 @@ main(
 	while (iocur_sp > start_iocur_sp)
 		pop_cur();
 	libxfs_umount(mp);
-	if (x.ddev)
-		libxfs_device_close(x.ddev);
-	if (x.logdev && x.logdev != x.ddev)
-		libxfs_device_close(x.logdev);
-	if (x.rtdev)
-		libxfs_device_close(x.rtdev);
-	libxfs_destroy();
+	libxfs_destroy(&x);
 
 	return exitcode;
 }
diff --git a/include/libxfs.h b/include/libxfs.h
index aaac00f6..504f6e9c 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -136,7 +136,7 @@ typedef struct libxfs_xinit {
 extern char	*progname;
 extern xfs_lsn_t libxfs_max_lsn;
 extern int	libxfs_init (libxfs_init_t *);
-extern void	libxfs_destroy (void);
+void		libxfs_destroy(struct libxfs_xinit *li);
 extern int	libxfs_device_to_fd (dev_t);
 extern dev_t	libxfs_device_open (char *, int, int, int);
 extern void	libxfs_device_close (dev_t);
diff --git a/libxfs/init.c b/libxfs/init.c
index 197690df..913f546f 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -259,6 +259,21 @@ destroy_zones(void)
 	return leaked;
 }
 
+static void
+libxfs_close_devices(
+	struct libxfs_xinit	*li)
+{
+	if (li->ddev)
+		libxfs_device_close(li->ddev);
+	if (li->logdev && li->logdev != li->ddev)
+		libxfs_device_close(li->logdev);
+	if (li->rtdev)
+		libxfs_device_close(li->rtdev);
+
+	li->ddev = li->logdev = li->rtdev = 0;
+	li->dfd = li->logfd = li->rtfd = -1;
+}
+
 /*
  * libxfs initialization.
  * Caller gets a 0 on failure (and we print a message), 1 on success.
@@ -385,12 +400,9 @@ libxfs_init(libxfs_init_t *a)
 		unlink(rtpath);
 	if (fd >= 0)
 		close(fd);
-	if (!rval && a->ddev)
-		libxfs_device_close(a->ddev);
-	if (!rval && a->logdev)
-		libxfs_device_close(a->logdev);
-	if (!rval && a->rtdev)
-		libxfs_device_close(a->rtdev);
+	if (!rval)
+		libxfs_close_devices(a);
+
 	return rval;
 }
 
@@ -913,9 +925,12 @@ libxfs_umount(
  * Release any global resources used by libxfs.
  */
 void
-libxfs_destroy(void)
+libxfs_destroy(
+	struct libxfs_xinit	*li)
 {
-	int	leaked;
+	int			leaked;
+
+	libxfs_close_devices(li);
 
 	/* Free everything from the buffer cache before freeing buffer zone */
 	libxfs_bcache_purge();
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1038e604..7f315d8a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3945,11 +3945,6 @@ main(
 	if (error)
 		exit(1);
 
-	if (xi.rtdev)
-		libxfs_device_close(xi.rtdev);
-	if (xi.logdev && xi.logdev != xi.ddev)
-		libxfs_device_close(xi.logdev);
-	libxfs_device_close(xi.ddev);
-	libxfs_destroy();
+	libxfs_destroy(&xi);
 	return 0;
 }
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index ccb13f4a..38578121 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -1111,12 +1111,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	if (error)
 		exit(1);
 
-	if (x.rtdev)
-		libxfs_device_close(x.rtdev);
-	if (x.logdev && x.logdev != x.ddev)
-		libxfs_device_close(x.logdev);
-	libxfs_device_close(x.ddev);
-	libxfs_destroy();
+	libxfs_destroy(&x);
 
 	if (verbose)
 		summary_report();

Re: [PATCH 2/2] libxfs: clean up libxfs_destroy

[Eric Sandeen]

On 2/24/20 4:11 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It's weird that libxfs_init opens the three devices passed in via the
> libxfs_xinit structure but libxfs_destroy doesn't actually close them.
> Fix this inconsistency and remove all the open-coded device closing.

Seems better.  Part of me wishes the device open/close weren't hidden
in init/destroy but from a quick glance that's harder to tease apart.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Re: [PATCH 2/2] libxfs: clean up libxfs_destroy

[Brian Foster]

On Mon, Feb 24, 2020 at 04:11:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It's weird that libxfs_init opens the three devices passed in via the
> libxfs_xinit structure but libxfs_destroy doesn't actually close them.
> Fix this inconsistency and remove all the open-coded device closing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  copy/xfs_copy.c     |    2 +-
>  db/init.c           |    8 +-------
>  include/libxfs.h    |    2 +-
>  libxfs/init.c       |   31 +++++++++++++++++++++++--------
>  mkfs/xfs_mkfs.c     |    7 +------
>  repair/xfs_repair.c |    7 +------
>  6 files changed, 28 insertions(+), 29 deletions(-)
> 
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index a6d67038..7f4615ac 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -1200,7 +1200,7 @@ main(int argc, char **argv)
>  
>  	check_errors();
>  	libxfs_umount(mp);
> -	libxfs_destroy();
> +	libxfs_destroy(&xargs);
>  
>  	return 0;
>  }
> diff --git a/db/init.c b/db/init.c
> index 0ac37368..e5450d2b 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -217,13 +217,7 @@ main(
>  	while (iocur_sp > start_iocur_sp)
>  		pop_cur();
>  	libxfs_umount(mp);
> -	if (x.ddev)
> -		libxfs_device_close(x.ddev);
> -	if (x.logdev && x.logdev != x.ddev)
> -		libxfs_device_close(x.logdev);
> -	if (x.rtdev)
> -		libxfs_device_close(x.rtdev);
> -	libxfs_destroy();
> +	libxfs_destroy(&x);
>  
>  	return exitcode;
>  }
> diff --git a/include/libxfs.h b/include/libxfs.h
> index aaac00f6..504f6e9c 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -136,7 +136,7 @@ typedef struct libxfs_xinit {
>  extern char	*progname;
>  extern xfs_lsn_t libxfs_max_lsn;
>  extern int	libxfs_init (libxfs_init_t *);
> -extern void	libxfs_destroy (void);
> +void		libxfs_destroy(struct libxfs_xinit *li);
>  extern int	libxfs_device_to_fd (dev_t);
>  extern dev_t	libxfs_device_open (char *, int, int, int);
>  extern void	libxfs_device_close (dev_t);
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 197690df..913f546f 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -259,6 +259,21 @@ destroy_zones(void)
>  	return leaked;
>  }
>  
> +static void
> +libxfs_close_devices(
> +	struct libxfs_xinit	*li)
> +{
> +	if (li->ddev)
> +		libxfs_device_close(li->ddev);
> +	if (li->logdev && li->logdev != li->ddev)
> +		libxfs_device_close(li->logdev);
> +	if (li->rtdev)
> +		libxfs_device_close(li->rtdev);
> +
> +	li->ddev = li->logdev = li->rtdev = 0;
> +	li->dfd = li->logfd = li->rtfd = -1;
> +}
> +
>  /*
>   * libxfs initialization.
>   * Caller gets a 0 on failure (and we print a message), 1 on success.
> @@ -385,12 +400,9 @@ libxfs_init(libxfs_init_t *a)
>  		unlink(rtpath);
>  	if (fd >= 0)
>  		close(fd);
> -	if (!rval && a->ddev)
> -		libxfs_device_close(a->ddev);
> -	if (!rval && a->logdev)
> -		libxfs_device_close(a->logdev);
> -	if (!rval && a->rtdev)
> -		libxfs_device_close(a->rtdev);
> +	if (!rval)
> +		libxfs_close_devices(a);
> +
>  	return rval;
>  }
>  
> @@ -913,9 +925,12 @@ libxfs_umount(
>   * Release any global resources used by libxfs.
>   */
>  void
> -libxfs_destroy(void)
> +libxfs_destroy(
> +	struct libxfs_xinit	*li)
>  {
> -	int	leaked;
> +	int			leaked;
> +
> +	libxfs_close_devices(li);
>  
>  	/* Free everything from the buffer cache before freeing buffer zone */
>  	libxfs_bcache_purge();
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1038e604..7f315d8a 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3945,11 +3945,6 @@ main(
>  	if (error)
>  		exit(1);
>  
> -	if (xi.rtdev)
> -		libxfs_device_close(xi.rtdev);
> -	if (xi.logdev && xi.logdev != xi.ddev)
> -		libxfs_device_close(xi.logdev);
> -	libxfs_device_close(xi.ddev);
> -	libxfs_destroy();
> +	libxfs_destroy(&xi);
>  	return 0;
>  }
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index ccb13f4a..38578121 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -1111,12 +1111,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
>  	if (error)
>  		exit(1);
>  
> -	if (x.rtdev)
> -		libxfs_device_close(x.rtdev);
> -	if (x.logdev && x.logdev != x.ddev)
> -		libxfs_device_close(x.logdev);
> -	libxfs_device_close(x.ddev);
> -	libxfs_destroy();
> +	libxfs_destroy(&x);
>  
>  	if (verbose)
>  		summary_report();
> 

Re: [PATCH 2/2] libxfs: clean up libxfs_destroy

[Christoph Hellwig]

On Mon, Feb 24, 2020 at 04:11:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It's weird that libxfs_init opens the three devices passed in via the
> libxfs_xinit structure but libxfs_destroy doesn't actually close them.
> Fix this inconsistency and remove all the open-coded device closing.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>