Re: [PATCH v2 2/5] btrfs: initial fsverity support

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Mar 15, 2021 at 05:42:48PM -0700, Boris Burkov wrote:
> > Is the separate size field really needed?  It seems like the type of thing that
> > would be redundant with information that btrfs already stores about the tree
> > items.  Having two sources of truth is error-prone.
> 
> For the case where the user supplied signature is absent or fits in one
> 1K btrfs verity descriptor item, I believe you are right, and we could
> infer that from the generic item size. Assuming arbitrary signature
> lengths, we could still probably do it by searching for the last item
> and computing the size from its key offset and item size. Though now
> that we have a special meaning for offset 0, that gets a bit messier.
> 
> I believe we will need the special item no matter what for the eventual
> encryption case, so I figured it wasn't a big deal to include the size
> rather than try to come up with some computation to get it. I like the
> argument about redundant representation, and saving 8 bytes per verity
> enabled file isn't nothing either. I will see if I can compute it
> efficiently without adding the extra field.

To be clear, I'm not necessarily saying "remove the size field".  But if you
keep it, it needs a proper explanation and careful consideration of what to do
if it doesn't match the actual size.

I don't think 8 bytes per file matters, especially when the Merkle tree blocks
are zero-padded to 4096 bytes anyway.

> > > +	true_size = btrfs_stack_verity_descriptor_size(&item);
> > > +	if (!buf_size)
> > > +		return true_size;
> > > +	if (buf_size < true_size)
> > > +		return -ERANGE;
> > 
> > 
> > It would be good to validate that true_size <= INT_MAX, as it's returned it an
> > 'int'.
> > 
> > > +
> > > +	ret = read_key_bytes(BTRFS_I(inode),
> > > +			     BTRFS_VERITY_DESC_ITEM_KEY, 1,
> > > +			     buf, buf_size, NULL);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	if (ret != buf_size)
> > > +		return -EIO;
> > > +
> > > +	return buf_size;
> > 
> > Shouldn't this part use true_size, not buf_size?
> 
> I felt this was clunky when writing it. If I'm not mistaken, it doesn't
> really matter what we return since the actual key line is the
> `ret != buf_size` check above.
> 
> I think it would be reasonable to allow too-large buffers and check
> against/return true_size. If true_size is somehow wrong, then presumably
> the signature check itself will still fail.

fsverity_operations::get_verity_descriptor() is supposed to behave like
getxattr().  That means that if the buffer provided is too large, the function
shouldn't fail but rather just read and return the actual size.

That means using true_size above.

I probably should have gone with something harder to get wrong, but getxattr()
semantics seemed "simple".

- Eric