linux-hardening - Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2022 11:45:19 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        xfs <linux-xfs@...r.kernel.org>, Zorro Lang <zlang@...hat.com>,
        linux-hardening@...r.kernel.org
Subject: Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item
 memcpy

On Tue, Oct 25, 2022 at 09:32:35AM +1100, Dave Chinner wrote:
> On Mon, Oct 24, 2022 at 09:59:08AM -0700, Kees Cook wrote:
> > On Wed, Oct 19, 2022 at 05:04:11PM -0700, Darrick J. Wong wrote:
> > > [...]
> > > -/*
> > > - * Copy an BUI format buffer from the given buf, and into the destination
> > > - * BUI format structure.  The BUI/BUD items were designed not to need any
> > > - * special alignment handling.
> > > - */
> > > -static int
> > > -xfs_bui_copy_format(
> > > -	struct xfs_log_iovec		*buf,
> > > -	struct xfs_bui_log_format	*dst_bui_fmt)
> > > -{
> > > -	struct xfs_bui_log_format	*src_bui_fmt;
> > > -	uint				len;
> > > -
> > > -	src_bui_fmt = buf->i_addr;
> > > -	len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
> > > -
> > > -	if (buf->i_len == len) {
> > > -		memcpy(dst_bui_fmt, src_bui_fmt, len);
> > > -		return 0;
> > > -	}
> > > -	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > > -	return -EFSCORRUPTED;
> > > -}
> > 
> > This is the place where flex_cpy() could be used:
> > 
> > 	flex_cpy(dst_bui_fmt, src_bui_fmt);
> 
> How does flex_cpy() know how much memory was allocated for
> dst_bui_fmt? Doesn't knowing this imply that we have to set the
> count field in dst_bui_fmt appropriately before flex_cpy() is
> called?

Right -- this is why I had originally sent my API proposal with the *_dup
helpers included as well. The much more common case is allocate/copy
and allocate/deserialize. The case of doing flex-to-flex is odd, because
it implies there was an external allocation step, etc. But, that said,
allocation and bounds recording are usually pretty well tied together.

> Hence I don't see that this flex array copying stuff will make it
> harder to make mistakes, but ISTM that it'll make them harder to spot
> during review and audit...

I think the transition to a fallible routine is an improvement, but
yes, I expect flex_cpy() not to be used much compared to flex_dup(),
or mem_to_flex_dup(), both of which collapse a great many steps and
sanity checks into a single common, internally-consistent, and fallible
operation.

-- 
Kees Cook