| Message ID | 158258948007.451256.11063346596276638956.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxfs: minor cleanups of destructors | expand |
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; > } >
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; > > } > >
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; > } >
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.
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
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
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; }