Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <53d6b712-e2e6-9188-51c7-6190a42dcc5f@amd.com>
Date:   Tue, 15 Aug 2023 14:51:47 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation
 reports
Content-Language: en-US
To:     Dan Williams <dan.j.williams@intel.com>, linux-coco@lists.linux.dev
Cc:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Dionna Amalie Glaze <dionnaglaze@google.com>,
        James Bottomley <James.Bottomley@HansenPartnership.com>,
        Peter Gonda <pgonda@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Samuel Ortiz <sameo@rivosinc.com>, peterz@infradead.org,
        x86@kernel.org, linux-kernel@vger.kernel.org
References: <169199898909.1782217.10899362240465838600.stgit@dwillia2-xfh.jf.intel.com>
 <169199900110.1782217.12112722316618515086.stgit@dwillia2-xfh.jf.intel.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <169199900110.1782217.12112722316618515086.stgit@dwillia2-xfh.jf.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aTkySWF3SVpYQ29KTnExbkRyQWQra1laakZVV003eEZyaDc0RlZNbTZzaDRW?=
 =?utf-8?B?KzBzZ0ZmUWZVOXJtTm9GUEhnY2VZa1R3eVp6ZStlMkxhZVdCZytXMFFhcE83?=
 =?utf-8?B?U1V1WDlYSjFmZ0VtSUtSaG1ZUEIyenZPK3FWMHZVWnNOcmNqSkRtWjczSTBD?=
 =?utf-8?B?dkh0MnZhVldOU1dpQ0hFZDI5eS9mdXdNejRrcjRQYktjeGoxa2hML01BK3pK?=
 =?utf-8?B?QmNBdUN4LytVZEg3dEZrZGl4Y3hVaDVGb0dtNWErU3BCc3Vjb2pYL0VHeXNt?=
 =?utf-8?B?RjVNaGIxY014bFdBWExTYW8zd1VSblJ6WmpuLzZBQThQdWpCTTZiNjkxVGEz?=
 =?utf-8?B?NjltUWNOWE1Sd2wwOUpHRmRtclM1VUlpbkRLTW9RTW8zazBPL3d6NnZHV2Ix?=
 =?utf-8?B?c1J1Wmh6aE5OUSs3QXlQQVRCUTRPWHl3TDdVcXdXYzYyK0xNNUtrSmI2MXk1?=
 =?utf-8?B?YU9tVEdoem5mdC9iay9xWE1OSVc2QjJFSVYxTjA2VmJlOStlRTJQVjIzM25H?=
 =?utf-8?B?UHhRMUhsVjk2N1JWTGZVZlFVdFpOblFTRHF1Rmw1ZVBjQlg1SVFCdXZudVZR?=
 =?utf-8?B?dnJmZm4wQUhub0lDUTlKNVY1bVZ4aEIwazVzTzhDWVRFRjlteTRkTlVYUitx?=
 =?utf-8?B?cWtRMUVTRmRHTEZ6b2FSOWpHVWp6MFNLVXZnR0RnS2VkWFZiQWIxc3FPV2pY?=
 =?utf-8?B?MW9wcEtvb0ZDZ2JreVRMd29uM3F1NGJJVE9JQWVIRXlMaS9nNnFyZDVjSEh5?=
 =?utf-8?B?Z0hoT2xaU0NGMU5WZFo4TEtGRjd0WGd6cThQZlNtL2Y0Z3F6RWh5YkxhbUpz?=
 =?utf-8?B?NkFCSUNFMXdGcjM1QmtzUCs2UlB3cW5CdEdUMjRjRzJRclZLZlVkVVBlelJa?=
 =?utf-8?B?dktwYjh3Y08wd091UDNMRjNSS2plc24ya1RMZUxjWmlzazA1M1ZyTkJHWW1k?=
 =?utf-8?B?YmNMaklzYW03YloxY2xzQTRIenFhOTZndmMvRDBJRDIzM3BHdG1LWUpLUXpR?=
 =?utf-8?B?OWJsYWFFSlpvNjFwKzNTOElCYUM5NkJicnhlUzFyd1A3dFd4Y2NFZFM3a2pV?=
 =?utf-8?B?Yjh2eHhGS3BjSWh3SlczblVJWVdhbmdVOVVoZlpVeElwaUZaTFR0L0lHTEZZ?=
 =?utf-8?B?SDlHWFZaV1VHcUNOM1ZFZlZVektldDlNL0FhZFVPVXBGZnRaVnhqbEtITFRP?=
 =?utf-8?B?WVFjVGNNakFNWUdRMUZFTjlKalVEMmJnNUZ4ZWhmcXRXVXYveEhqZXNmNURS?=
 =?utf-8?B?Um5PNFZZOWNNRDZ3NVBscUtjR1lkNk90NmJkTGVVVVNWY2creHcyVUcwQkNx?=
 =?utf-8?B?ZVVxbnFrMlhRSXFTdGJEY25lNEpLYUZycngvZ25pdWQvZFhuWk42YU1JYTI1?=
 =?utf-8?B?UzlSM2ZOTDJWbm41a3ZDc2hteHV2VWYxQTBZWDQ5bVlIb0lZbHdMdENsOTZM?=
 =?utf-8?B?S09ZL3ArRE1XRlpreDRRcTByd0ZiN1ZoVkUzU3NoZ2hVZk10MDdaTEJUMGkv?=
 =?utf-8?B?SWxBNUlxWTdhWEFhMHdPNG9adHlieGlCU04xZHlEd2dKL2FUSTNxVlNVUEs1?=
 =?utf-8?B?VTZHNzE0b0JaTnJCcVBNVVFOMVZzRzAzZUw5TnV1TDl6U212bG5IS1gvTklR?=
 =?utf-8?B?cEtYdlR4MllXemU4QmthaGwrQmJmdEhVa0poU2ptbHphSFg2dWhqYTZWZFlZ?=
 =?utf-8?B?OXB5RThpNDN0WTVnUGZraGV0cjBNMURyS1BscVM3QWg4WTZ1SFVJNGxuRS80?=
 =?utf-8?B?UXc3cm9oNTlFNEpSb0pNaXNoYmxWejAxRC9HdDM1RWZBUTFsaXdjUlFqaUJX?=
 =?utf-8?B?VGhIWnZMK2UxVitXNC9hZjVsWTJrbC9udWJIZG1UMDNlZzd0Y1lNWXFQK1d3?=
 =?utf-8?B?WWlReGVqZk9obk5wcTlFcndOODhDZUpIckxYaDVzVUlIQ1dyUjdoSTlZUy9V?=
 =?utf-8?B?K0tXSERSV09JWHU0T3lQcEZTUnlCbGFiWmZLM1cxWDRTdUR3elNNS25wY1d2?=
 =?utf-8?B?L2NKeXlHU1dqVHRvenRqaG94WVFKUTlNc0NndWZ6ZFMzMDA3a21wTlFpYXQ2?=
 =?utf-8?B?T0FOUmJqSHJBK2pSN1lsMVBNUHNnMzFldnFZdXFrVU0vV2xvRlBLdjU0eDlV?=
 =?utf-8?Q?Fk005ua8/S4Ivp8ljD0ayFd6Z?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b595433-9bb6-4f95-15d9-08db9dc910a2
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2023 19:51:49.7743
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iw5RU8E568G9zfpWASfFDztruVL6m0ysTzzeB50KDS9ySJ+CG30wzhEEXfqQKaA0LlnLbASeRWE3dyYorhC01Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7189
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 8/14/23 02:43, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its launch state, sign it and
> submit it to a verifying party. Upon successful attestation that
> verifies the integrity of the TVM additional secrets may be deployed.
> The concept is common across TSMs, but the implementations are
> unfortunately vendor specific. While the industry grapples with a common
> definition of this attestation format [1], Linux need not make this
> problem worse by defining a new ABI per TSM that wants to perform a
> similar operation. The current momentum has been to invent new ioctl-ABI
> per TSM per function which at best is an abdication of the kernel's
> responsibility to make common infrastructure concepts share common ABI.
> 
> The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> more, is to define a sysfs interface to retrieve the TSM-specific blob.
> 
>      echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex
>      hexdump /sys/class/tsm/tsm0/outblob
> 
> This approach later allows for the standardization of the attestation
> blob format without needing to change the Linux ABI. Until then, the
> format of 'outblob' is determined by the parent device for 'tsm0'.
> 
> The expectation is that this is a boot time exchange that need not be
> regenerated, making it amenable to a sysfs interface. In case userspace
> does try to generate multiple attestation reports it includes conflict
> detection so userspace can be sure no other thread changed the
> parameters from its last configuration step to the blob retrieval.
> 
> TSM specific options are encoded as 'extra' attributes on the TSM device
> with the expectation that vendors reuse the same options for similar
> concepts. The current options are defined by SEV-SNP's need for a
> 'privilege level' concept (VMPL), and the option to retrieve a
> certificate chain in addition to the attestation report ("extended"
> format).
> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-class-tsm |   47 +++++
>   MAINTAINERS                               |    8 +
>   drivers/virt/coco/Kconfig                 |    4
>   drivers/virt/coco/Makefile                |    1
>   drivers/virt/coco/tdx-guest/Kconfig       |    1
>   drivers/virt/coco/tsm.c                   |  290 +++++++++++++++++++++++++++++
>   include/linux/tsm.h                       |   45 +++++
>   7 files changed, 396 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-tsm
>   create mode 100644 drivers/virt/coco/tsm.c
>   create mode 100644 include/linux/tsm.h
> 


> +/*
> + * Input is a small hex blob, rather than a writable binary attribute, so that
> + * it is conveyed atomically.
> + */
> +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t len)
> +{
> +	u8 inblob[TSM_INBLOB_MAX];
> +	size_t inblob_len;
> +	int rc;
> +
> +	inblob_len = len;
> +	if (buf[len - 1] == '\n')
> +		inblob_len--;
> +	if (inblob_len & 1)
> +		return -EINVAL;
> +	inblob_len /= 2;
> +	if (inblob_len > TSM_INBLOB_MAX)
> +		return -EINVAL;

Is an array_index_nospec() call needed after this check?

> +
> +	rc = hex2bin(inblob, buf, inblob_len);
> +	if (rc < 0)
> +		return rc;
> +
> +	guard(rwsem_write)(&tsm_rwsem);
> +	if (memcmp(tsm_report.desc.inblob, inblob, inblob_len) == 0)
> +		return len;
> +	memcpy(tsm_report.desc.inblob, inblob, inblob_len);

Should the portion of tsm_report.desc.inblob that is not updated be 
cleared if inblob_len != TSM_INBLOB_MAX?

> +	tsm_report.desc.inblob_len = inblob_len;
> +	tsm_report.write_generation++;
> +
> +	return len;
> +}
> +


> +int register_tsm(const struct tsm_ops *ops, struct device *parent,
> +		 const struct attribute_group **groups)
> +{
> +	const struct tsm_ops *conflict;
> +	struct device *dev;
> +	int rc;
> +
> +	if (!parent)
> +		return -EINVAL;
> +
> +	if (!groups)
> +		groups = tsm_default_attribute_groups;
> +
> +	guard(rwsem_write)(&tsm_rwsem);
> +	conflict = provider.ops;
> +	if (conflict) {
> +		pr_err("\"%s\" ops already registered\n", conflict->name);
> +		return rc;

Looks like rc is used uninitialized.

> +	}
> +
> +	dev = device_create_with_groups(tsm_class, parent, 0, NULL, groups,
> +					"tsm0");

You can go out to 100 characters now, so this could all be one line.

Thanks,
Tom

> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	provider.ops = ops;
> +	provider.dev = dev;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_tsm);
> +