Re: [PATCH 1/8] xfs: hide log iovec alignment constraints

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Apr 27, 2022 at 02:50:34PM +1000, Dave Chinner wrote:
> On Tue, Apr 26, 2022 at 08:14:45PM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 27, 2022 at 12:22:52PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Callers currently have to round out the size of buffers to match the
> > > aligment constraints of log iovecs and xlog_write(). They should not
> > > need to know this detail, so introduce a new function to calculate
> > > the iovec length (for use in ->iop_size implementations). Also
> > > modify xlog_finish_iovec() to round up the length to the correct
> > > alignment so the callers don't need to do this, either.
> > > 
> > > Convert the only user - inode forks - of this alignment rounding to
> > > use the new interface.
> > 
> > Hmm.  So currently, we require that the inode fork buffer be rounded up
> > to the next 4 bytes, and then I guess the log will copy that into the
> > log iovec?  IOWs, if we have a 37-byte data fork, we'll allocate a 40
> > byte buffer for the xfs_ifork, and the log will copy all 40 bytes into a
> > 40 byte iovec.
> 
> Yes, that's how the current code works. It ends up leaking whatever
> was in those 3 bytes into the shadow buffer that we then copy into
> the log region. i.e. the existing code "leaks" non-zeroed allocated
> memory to the journal.
> 
> > Now it looks like we'd allocate a 37-byte buffer for the xfs_ifork, but
> > the log iovec will still be 40 bytes.  So ... do we copy 37 bytes out of
> > the ifork buffer and zero the last 3 bytes in the iovec?
> 
> Yes, we copy 37 bytes out of the ifork buffer now into the shadow
> buffer so we do not overrun the inode fork buffer.
> 
> > Does we leak
> > kernel memory in those last 3 bytes?
> 
> We does indeed still leak the remaining 3 bytes as they are not
> zeroed.
> 
> > Or do we copy 40 bytes and
> > overrun?
> 
> No, we definitely don't do that - KASAN gets very unhappy when you
> do that...
> 
> > It sorta looks like (at least for the local format case) xlog_copy_iovec
> > will copy 37 bytes and leave the last 3 bytes of the iovec in whatever
> > state it was in previously.  Is that zeroed?  Because it then looks like
> > xlog_finish_iovec will round that 37 up to 40.
> 
> The shadow buffer is only partially zeroed - the part that makes io
> the header and iovec pointer array is zeroed, but the region that
> the journal data is written to is not zeroed.
> 
> > (FWIW I'm just checking for kernel memory exposure vectors here.)
> 
> Yup, I hadn't even considered that aspect of the code because we
> aren't actually leaking anything to userspace. If an unprivileged
> user can read 3 bytes of uninitialised data out of the journal we've
> got much, much bigger security problems to deal with.
> 
> It should be trivial to fix, though. I'll do the initial fix as a
> standalone patch, though, and then roll it into this one because the
> problem has been around for a long while and fixing this patch
> doesn't produce an easily backportable fix...

<nod> I agree that it's a very minor disclosure vulnerability (certainly
less severe than ALLOCSP) since you'd need CAP_SYS_RAWIO to exploit it.
But certainly worth patching before someone discovers that a former
pagecache page with your credit card numbers on it got recycled into a
log vector page.  Thanks for doing the fix. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com