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
