Re: [PATCH v10 5/7] virt: coco: arm-cca-guest: Rename TSM report source file

[Jason Gunthorpe <jgg@nvidia.com>]

> The Arm CCA guest driver currently only implements TSM report support, but
> follow-up changes will add more TSM-related functionality to the same
> module.
> 
> Rename arm-cca-guest.c to main.c and build it as an object of the
> arm-cca-guest module. This leaves room for the module to grow additional
> source files.

It doesn't matter but I would have described this as:

If a module is comprised of a single .c file with the same name as the
module then kbuild does not permit adding any more .c files to the
module. Rename arm-cca-guest.c to main.c so more source files can be
added.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

> [ ... 19 lines skipped ... ]
> @@ -156,7 +156,7 @@ static int arm_cca_report_new(struct tsm_report *report, void *data)
>  	return ret;
>  }

    [Severity: High]
    This isn't a bug introduced by this patch, but while reviewing the renamed
    arm_cca_report_new() function, I noticed a potential out-of-bounds read
    involving the RMM response length.

    Does this code properly validate the length returned by the Realm Management
    Monitor (RMM) before copying data?

    In arm_cca_attestation_continue(), the untrusted length returned by the RMM
    is unconditionally added to info->offset:

	info->offset += len;

    If a buggy or compromised RMM returns an unexpectedly large length,
    info.offset can exceed RSI_GRANULE_SIZE (4096 bytes). The loop in
    arm_cca_report_new() will terminate since info.offset < RSI_GRANULE_SIZE
    becomes false, but as long as token_size + info.offset <= max_size, it
    executes this copy:

	memcpy(&token[token_size], buf, info.offset);

    Since buf is allocated to be exactly RSI_GRANULE_SIZE bytes, using an
    unvalidated info.offset here results in an out-of-bounds read from buf.

    Could this leak adjacent kernel heap memory into the attestation token
    that is returned to userspace?

This seems like something that should be fixed independently for
robustness.

-- 
Jason