Re: [PATCH v2 3/3] virt: arm-cca-guest: Add support for measurement registers

[Yeoreum Yun]

> On 09/30 07:40, Yeoreum Yun wrote:
> > > > > > +/**
> > > > > > + * arm_cca_measurements - ARM CCA measurement configuration instance.
> > > > > > + *
> > > > > > + * This defines the measurement set and behavior for the ARM
> > > > > > + * Confidential Compute Architecture, enabling measurements
> > > > > > + * for attestation and runtime validation.
> > > > > > + */
> > > > > > +static struct tsm_measurements arm_cca_measurements = {
> > > > > > +	.mrs = arm_cca_mrs,
> > > > > > +	.nr_mrs = ARRAY_SIZE(arm_cca_mrs),
> > > > > > +	.refresh = arm_cca_mr_refresh,
> > > > > > +	.write = arm_cca_mr_extend,
> > > > > 
> > > > > From the RMM specification and the commit message, I understand that a
> > > > > REM can be extended with a measurement value of up to 64 bytes,
> > > > > regardless of the selected hash algorithm (for example, SHA-256 or
> > > > > SHA-512).
> > > > > 
> > > > > However, when SHA-256 is selected, this interface does not allow a REM
> > > > > to be extended with a 64-byte value. The write partially succeeds: the
> > > > > REM is extended with the first 32 bytes, and then the write of the
> > > > > remaining 32 bytes fails with -EFBIG.
> > > > > 
> > > > > As far as I can tell, the write is truncated to the sysfs binary
> > > > > attribute size, which is set to the SHA-256 digest size (32 bytes). The
> > > > > subsequent write at offset 32 is then rejected by sysfs_kf_bin_write()
> > > > > with -EFBIG.
> > > > > 
> > > > > I do not see a straightforward fix because the TSM measurement register
> > > > > interface currently uses mr_size both as the readable digest size and as
> > > > > the required write size.
> > > > > 
> > > > > [Realm VM]
> > > > >   ~ # export REM3=/sys/devices/virtual/misc/arm_cca_guest/measurements/rem3:sha256
> > > > >   ~ # dd if=/dev/urandom bs=64 count=1 of=$REM3
> > > > >   dd: error writing '/sys/devices/virtual/misc/arm_cca_guest/measurements/rem3:sha256': File too large
> > > > >   1+0 records in
> > > > >   0+0 records out
> > > > > 
> > > > > [RMM]
> > > > >   SMC_RSI_MEASUREMENT_EXTEND        4 20 3cfcaa635bca042d c148121346b6e1b5 7342800b438d20d7 3dc7a25048cbbad8 0 0 0 0 > RSI_SUCCESS
> > > > > 
> > > > > Do you have any thoughts on how the TSM interface should represent the
> > > > > maximum extend input size separately from the digest size?
> > > > 
> > > > I don't think we need to represent the maximum extend input size separately.
> > > > 
> > > > The TSM measurement register interface is intended to expose PCR-like
> > > > semantics, where the value being extended is a measurement digest whose size
> > > > is determined by the selected hash algorithm.
> > > > 
> > > > In other words, the extend operation is conceptually:
> > > > 
> > > > new_digest = Hash(old_digest || measurement_digest)
> > > > 
> > > > Therefore, for a SHA-256 measurement register, the extend value should be
> > > > 32 bytes, and rejecting a value larger than 32 bytes with -EFBIG seems
> > > > correct to me and intended. A 64-byte extend value would only be valid for
> > > > a register using a 64-byte digest, such as SHA-512.
> > > > 
> > > > Although the RMM interface may allow a measurement value of up to 64 bytes
> > > > independently of the REM hash algorithm, I don't think that capability
> > > > needs to be exposed through the generic TSM measurement register interface
> > > > in point of viewt to preserve PCR-like semantics.
> > > 
> > > Thanks for the clarification. That makes sense to me.
> > > 
> > > In that case, could the ABI documentation be updated? It currently
> > > states:
> > >   All writes must start at offset 0 and be maximum 64 bytes in size.
> > >   Attempting to write more than 64 bytes will result in EINVAL returned
> > >   by the write() syscall.
> > 
> > Yeap. I'll update accordingly. Thanks.
> > 
> > > 
> > > However, the TSM interface requires the write size to exactly match
> > > mr_size. Therefore, this would be 32 bytes for SHA-256, 48 bytes for
> > > SHA-384, and 64 bytes for SHA-512.
> > > 
> > > My remaining concern is that since sysfs_kf_bin_write() truncates
> > > oversized writes to the binary attribute size before invoking the
> > > callback, tm_digest_write() sees an exact-sized write and extends the
> > > MR. The following validation is useless in this case.
> > > 
> > >   static ssize_t tm_digest_write(struct file *filp, struct kobject *kobj,
> > >   			       const struct bin_attribute *attr, char *buffer,
> > >   			       loff_t off, size_t count)
> > >   {
> > >     [...]
> > >   	/* partial writes are not supported */
> > >   	if (off != 0 || count != attr->size)
> > >   		return -EINVAL;
> > > 
> > > IMO this is not specific to Arm CCA, but do you have any thoughts on
> > > this?
> > 
> > I think this is ultimately a limitation of sysfs. At this layer,
> > simply knowing that userspace supplied a larger buffer does not allow us to
> > determine whether all of the data in that buffer is valid.
> 
> I agree that this is a common sysfs/TSM issue rather than something
> that needs to be addressed in this series.
> 
> > 
> > For example, a userspace program could allocate a 64-byte buffer,
> > place only a 32-byte SHA-256 digest in it, and still pass the full buffer
> > size to write(). From the kernel's point of view, there is no reliable way
> > to distinguish that from a valid 64-byte input.
> > 
> > Therefore, I think userspace needs to check the size of the binary attribute,
> > e.g. remX256 in this case, and write exactly that amount of valid data.
> 
> Agreed. Userspace should normally inspect the attribute size and write
> exactly that mount.
> 
> > 
> > Given the current sysfs interface, I think requiring userspace to check
> > the bin_attr size and provide valid data of exactly that size is the best
> > we can do.
> > 
> > IOW, above sanity check is to catch-up what you worried about as comment
> > say, Its purpose to prohibit the *partial write*.
> 
> Right, but my concern is that this check cannot detect an oversized
> write, since sysfs_kf_bin_write() truncates it before invoking
> tm_digest_write(). Consequently, the check passes and the MR is
> extended, even though userspace observes a short write.
> 
> I think this could be handled by adding an opt-in option that prevents
> sysfs from truncating writes. I did a basic test with the prototype
> patch below and confirmed that an oversized digest was rejected with
> -EFBIG before the MR was extended.
> 
> Anyway, I will look into addressing this separately through the common
> TSM/sysfs code. 
> 
> Thanks for the clarification, Yeoreum.
> 
> ---8<---
> diff --git a/drivers/virt/coco/guest/tsm-mr.c b/drivers/virt/coco/guest/tsm-mr.c
> index 657b9c5739d0..5880b12e1f55 100644
> --- a/drivers/virt/coco/guest/tsm-mr.c
> +++ b/drivers/virt/coco/guest/tsm-mr.c
> @@ -215,6 +215,7 @@ tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
>                 if (tm->mrs[i].mr_flags & TSM_MR_F_WRITABLE) {
>                         bap->attr.mode |= 0200;
>                         bap->write = tm_digest_write;
> +                       bap->no_write_truncate = true;
>                 }
> 
>                 bap->size = tm->mrs[i].mr_size;
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index cd5bb0f9fee6..1d6a0bf8109e 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -156,6 +156,8 @@ static ssize_t sysfs_kf_bin_write(struct kernfs_open_file *of, char *buf,
>         if (size) {
>                 if (size <= pos)
>                         return -EFBIG;
> +               if (battr->no_write_truncate && count > size - pos)
> +                       return -EFBIG;
>                 count = min_t(ssize_t, count, size - pos);
>         }
>         if (!count)
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index b1a3a1e6ad09..31a4c3cf5d51 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -312,6 +312,7 @@ struct bin_attribute {
>         struct attribute        attr;
>         size_t                  size;
>         void                    *private;
> +       bool                    no_write_truncate;
>         struct address_space *(*f_mapping)(void);
>         ssize_t (*read)(struct file *, struct kobject *, const struct bin_attribute *,
>                         char *, loff_t, size_t);
> 

Yeap. I agree this and valid for me, but delay decision to sysfs
maintainer whether it's valid for overall usage of sysfs.

-- 
Sincerely,
Yeoreum Yun