[PATCH 2/3] crypto: ccp - do not start PSP sub-devices without their vdata

[PATCH 2/3] crypto: ccp - do not start PSP sub-devices without their vdata

[Mattia Tadini]

psp_init() decides whether to bring up SEV and TEE from the hardware
capability register alone:

	if (!psp_check_tee_support(psp)) {
		ret = tee_dev_init(psp);
		if (ret)
			return ret;
	}

tee_dev_init() then needs psp->vdata->tee, and fails with "tee: missing
driver data" when the vdata does not carry it. That failure is fatal to
psp_init(), so platform access, DBC and HSTI are lost along with it even
though none of them depends on the TEE.

Require both the hardware capability and the driver data before starting
a sub-device, so a psp_vdata that deliberately omits SEV or TEE gets the
rest of the PSP rather than nothing at all.

Signed-off-by: Mattia Tadini <info@mtsistemi.it>
---
 drivers/crypto/ccp/psp-dev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index b14ce51..c9ab47c 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -198,13 +198,17 @@ static int psp_init(struct psp_device *psp)
 {
 	int ret;
 
-	if (!psp_check_sev_support(psp)) {
+	/*
+	 * The capability register describes the silicon and the vdata describes
+	 * what this driver was given to drive it with. Require both.
+	 */
+	if (psp->vdata->sev && !psp_check_sev_support(psp)) {
 		ret = sev_dev_init(psp);
 		if (ret)
 			return ret;
 	}
 
-	if (!psp_check_tee_support(psp)) {
+	if (psp->vdata->tee && !psp_check_tee_support(psp)) {
 		ret = tee_dev_init(psp);
 		if (ret)
 			return ret;
-- 
2.55.0

[PATCH 3/3] crypto: ccp - add support for the AMD BC-250 secure processor

[Mattia Tadini]

The AMD BC-250 (Cyan Skillfish, a Zen 2 APU) carries an AMD Secure
Processor at PCI 1022:143e which no entry in sp_pci_table[] matches, so
the device is left unbound with its memory windows disabled.

The register layout was read off the device rather than assumed. With the
memory decode enabled and BAR 2 mapped read-only:

  CCP version  0x00100 = 0xFFFFFFFF   no CCP engine behind this function
  cmdresp      0x10544 = 0x80000000   pspv3/pspv4 mailbox, and it is live
  bootloader   0x109EC = 0x001C0102
  feature_reg  0x109FC = 0x00000002
  inten        0x10690 = 0x00000001
  pspv1 offsets            all zero   wrong layout
  pspv5-v7 offsets   all 0xFFFFFFFF   wrong layout

That is the pspv3/pspv4 layout. SEV is a server feature and is absent
here, and while the capability register advertises TEE, the ring never
comes up on this firmware:

	ccp 0000:01:00.2: tee: ring init command timed out, disabling TEE support

so the board gets a psp_vdata with platform access only. Platform access
uses its own mailbox (pa_v1, C2PMSG_28..30) and is unaffected by the
missing ring.

With this the PSP initialises and answers:

	ccp 0000:01:00.2: platform access enabled
	ccp 0000:01:00.2: psp enabled
	# cat /sys/bus/pci/devices/0000:01:00.2/bootloader_version
	00.1c.01.02

Dynamic boost control is kept in platform_features and probed. This
firmware rejects the command, which the driver already handles without
failing the probe:

	ccp 0000:01:00.2: msg 0x65 failed with PSP error: 0x4
	ccp 0000:01:00.2: dynamic boost control is unavailable

Signed-off-by: Mattia Tadini <info@mtsistemi.it>
---
 drivers/crypto/ccp/sp-pci.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
index f79df33..cf26b81 100644
--- a/drivers/crypto/ccp/sp-pci.c
+++ b/drivers/crypto/ccp/sp-pci.c
@@ -477,6 +477,21 @@ static const struct psp_vdata pspv7 = {
 	.intsts_reg		= 0x10514,	/* P2CMSG_INTSTS */
 };
 
+/*
+ * The BC-250 uses the pspv3 register layout but has no usable TEE: the
+ * capability register advertises one and PSP_CMD_TEE_RING_INIT never
+ * completes. Platform access has its own mailbox and is unaffected.
+ */
+static const struct psp_vdata pspv_bc250 = {
+	.platform_access	= &pa_v1,
+	.bootloader_info_reg	= 0x109ec,	/* C2PMSG_59 */
+	.feature_reg		= 0x109fc,	/* C2PMSG_63 */
+	.inten_reg		= 0x10690,	/* P2CMSG_INTEN */
+	.intsts_reg		= 0x10694,	/* P2CMSG_INTSTS */
+	.platform_features	= PLATFORM_FEATURE_DBC |
+				  PLATFORM_FEATURE_HSTI,
+};
+
 #endif
 
 static const struct sp_dev_vdata dev_vdata[] = {
@@ -547,6 +562,12 @@ static const struct sp_dev_vdata dev_vdata[] = {
 		.bar = 2,
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 		.psp_vdata = &pspv7,
+#endif
+	},
+	{	/* 10 */
+		.bar = 2,
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
+		.psp_vdata = &pspv_bc250,
 #endif
 	},
 
@@ -560,6 +581,7 @@ static const struct pci_device_id sp_pci_table[] = {
 	{ PCI_VDEVICE(AMD, 0x14CA), .driver_data = (kernel_ulong_t)&dev_vdata[5] },
 	{ PCI_VDEVICE(AMD, 0x15C7), .driver_data = (kernel_ulong_t)&dev_vdata[6] },
 	{ PCI_VDEVICE(AMD, 0x1649), .driver_data = (kernel_ulong_t)&dev_vdata[6] },
+	{ PCI_VDEVICE(AMD, 0x143e), .driver_data = (kernel_ulong_t)&dev_vdata[10] },
 	{ PCI_VDEVICE(AMD, 0x1134), .driver_data = (kernel_ulong_t)&dev_vdata[7] },
 	{ PCI_VDEVICE(AMD, 0x17E0), .driver_data = (kernel_ulong_t)&dev_vdata[7] },
 	{ PCI_VDEVICE(AMD, 0x156E), .driver_data = (kernel_ulong_t)&dev_vdata[8] },
-- 
2.55.0

[PATCH 1/3] crypto: ccp - fix NULL dereference in psp_firmware_is_visible()

[Mattia Tadini]

psp_firmware_is_visible() decides whether to publish tee_version by
testing the TEE capability bit reported by the hardware, and then reads
through the driver's own tee vdata pointer without checking it:

	if (attr == &dev_attr_tee_version.attr && psp->capability.tee &&
	    psp->vdata->tee->info_reg)

The capability register describes the silicon. The vdata describes what
this driver was given to drive it with. The two can disagree: any device
whose firmware sets the TEE capability bit while its psp_vdata carries no
tee data dereferences NULL here. The attribute group is registered from
probe, so the result is an oops during module init:

	RIP: 0010:psp_firmware_is_visible+0x6c/0x80 [ccp]
	 ? __pfx_init_module+0x10/0x10 [ccp]
	 sp_mod_init+0x1a/0xff0 [ccp]

This was hit on an AMD BC-250, whose PSP capability register at 0x109fc
reads 0x00000002 and so advertises a TEE that the board has no working
ring for.

Check the pointer before following it.

Fixes: 2e424c33d8e7 ("crypto: ccp - Add support for displaying PSP firmware versions")
Signed-off-by: Mattia Tadini <info@mtsistemi.it>
---
 drivers/crypto/ccp/sp-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
index ede6ff9..f79df33 100644
--- a/drivers/crypto/ccp/sp-pci.c
+++ b/drivers/crypto/ccp/sp-pci.c
@@ -80,7 +80,7 @@ static umode_t psp_firmware_is_visible(struct kobject *kobj, struct attribute *a
 		val = ioread32(psp->io_regs + psp->vdata->bootloader_info_reg);
 
 	if (attr == &dev_attr_tee_version.attr && psp->capability.tee &&
-	    psp->vdata->tee->info_reg)
+	    psp->vdata->tee && psp->vdata->tee->info_reg)
 		val = ioread32(psp->io_regs + psp->vdata->tee->info_reg);
 
 	/* If platform disallows accessing this register it will be all f's */
-- 
2.55.0

[PATCH 0/3] crypto: ccp - two PSP init fixes, and the AMD BC-250

[Mattia Tadini]

The AMD BC-250 is a Zen 2 APU board (Cyan Skillfish) sold as surplus mining
hardware and now used as a small desktop by a fair number of people. It carries
an AMD Secure Processor at PCI 1022:143e that sp_pci_table[] does not match, so
the device has always been left unbound with its memory windows disabled.

Adding the ID turned out to need two fixes first, both of the same shape: the
driver decides what to bring up from the hardware capability register alone,
and then uses the psp_vdata pointers for those features without checking that
this device has them. On this board the two disagree, because the firmware
advertises a TEE whose ring never comes up.

Patch 1 is a NULL pointer dereference in the sysfs is_visible callback. It
oopses during probe. It needs no new hardware to be reachable in principle:
any device whose firmware sets the TEE capability bit while its psp_vdata
carries no tee data hits it, which today means pspv1 and pspv2 parts. I have
only observed it on the BC-250, so I have not added a stable tag - please add
one if you think it deserves it.

Patch 2 stops a failed optional sub-device from taking down the rest of the
PSP. Without it, a psp_vdata that deliberately omits TEE gets nothing at all,
platform access included.

Patch 3 adds the board. The register layout was read off the device rather
than assumed, and the numbers are in the commit message.

Tested on a BC-250 running 7.2.6. Before:

  01:00.2 Encryption controller: AMD Device 143e
          Memory at fe700000 [disabled] [size=1M]
          Memory at fe884000 [disabled] [size=8K]
          (no driver)

After:

  ccp 0000:01:00.2: enabling device (0000 -> 0002)
  ccp 0000:01:00.2: platform access enabled
  ccp 0000:01:00.2: psp enabled
  # cat /sys/bus/pci/devices/0000:01:00.2/bootloader_version
  00.1c.01.02

with the platform mailbox answering commands. Dynamic boost control is probed
and cleanly rejected by this firmware, and HSTI reports nothing because the
security reporting capability bit is clear. There is no CCP crypto engine
behind this function: the version register at 0x100 reads back all ones.

The series is against v7.2.6 and touches only drivers/crypto/ccp/.

Mattia Tadini (3):
  crypto: ccp - fix NULL dereference in psp_firmware_is_visible()
  crypto: ccp - do not start PSP sub-devices without their vdata
  crypto: ccp - add support for the AMD BC-250 secure processor

 drivers/crypto/ccp/psp-dev.c |  8 ++++++--
 drivers/crypto/ccp/sp-pci.c  | 24 +++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.55.0

Re: [PATCH 1/3] crypto: ccp - fix NULL dereference in psp_firmware_is_visible()

[Mattia Tadini]

The cover letter for this series did not reach the list: the three
patches are in the archive, [PATCH 0/3] is not, and no bounce came back. Resent
here so the series context is in the thread rather than nowhere.

The AMD BC-250 is a Zen 2 APU board (Cyan Skillfish) sold as surplus mining
hardware and now used as a small desktop by a fair number of people. It carries
an AMD Secure Processor at PCI 1022:143e that sp_pci_table[] does not match, so
the device has always been left unbound with its memory windows disabled.

Adding the ID turned out to need two fixes first, both of the same shape: the
driver decides what to bring up from the hardware capability register alone,
and then uses the psp_vdata pointers for those features without checking that
this device has them. On this board the two disagree, because the firmware
advertises a TEE whose ring never comes up.

Patch 1 is a NULL pointer dereference in the sysfs is_visible callback. It
oopses during probe. It needs no new hardware to be reachable in principle:
any device whose firmware sets the TEE capability bit while its psp_vdata
carries no tee data hits it, which today means pspv1 and pspv2 parts. I have
only observed it on the BC-250, so I have not added a stable tag - please add
one if you think it deserves it.

Patch 2 stops a failed optional sub-device from taking down the rest of the
PSP. Without it, a psp_vdata that deliberately omits TEE gets nothing at all,
platform access included.

Patch 3 adds the board. The register layout was read off the device rather
than assumed, and the numbers are in the commit message.

Tested on a BC-250 running 7.2.6. Before:

  01:00.2 Encryption controller: AMD Device 143e
          Memory at fe700000 [disabled] [size=1M]
          Memory at fe884000 [disabled] [size=8K]
          (no driver)

After:

  ccp 0000:01:00.2: enabling device (0000 -> 0002)
  ccp 0000:01:00.2: platform access enabled
  ccp 0000:01:00.2: psp enabled
  # cat /sys/bus/pci/devices/0000:01:00.2/bootloader_version
  00.1c.01.02

with the platform mailbox answering commands. Dynamic boost control is probed
and cleanly rejected by this firmware, and HSTI reports nothing because the
security reporting capability bit is clear. There is no CCP crypto engine
behind this function: the version register at 0x100 reads back all ones.

The series is against v7.2.6 and touches only drivers/crypto/ccp/.

Mattia Tadini (3):
  crypto: ccp - fix NULL dereference in psp_firmware_is_visible()
  crypto: ccp - do not start PSP sub-devices without their vdata
  crypto: ccp - add support for the AMD BC-250 secure processor

 drivers/crypto/ccp/psp-dev.c |  8 ++++++--
 drivers/crypto/ccp/sp-pci.c  | 24 +++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.55.0

Re: [PATCH 0/3] crypto: ccp - two PSP init fixes, and the AMD BC-250

[Mario Limonciello]

On 9/19/26 13:34, Mattia Tadini wrote:
> The AMD BC-250 is a Zen 2 APU board (Cyan Skillfish) sold as surplus mining
> hardware and now used as a small desktop by a fair number of people. It carries
> an AMD Secure Processor at PCI 1022:143e that sp_pci_table[] does not match, so
> the device has always been left unbound with its memory windows disabled.
> 
> Adding the ID turned out to need two fixes first, both of the same shape: the
> driver decides what to bring up from the hardware capability register alone,
> and then uses the psp_vdata pointers for those features without checking that
> this device has them. On this board the two disagree, because the firmware
> advertises a TEE whose ring never comes up.
> 
> Patch 1 is a NULL pointer dereference in the sysfs is_visible callback. It
> oopses during probe. It needs no new hardware to be reachable in principle:
> any device whose firmware sets the TEE capability bit while its psp_vdata
> carries no tee data hits it, which today means pspv1 and pspv2 parts. I have
> only observed it on the BC-250, so I have not added a stable tag - please add
> one if you think it deserves it.
> 
> Patch 2 stops a failed optional sub-device from taking down the rest of the
> PSP. Without it, a psp_vdata that deliberately omits TEE gets nothing at all,
> platform access included.
> 
> Patch 3 adds the board. The register layout was read off the device rather
> than assumed, and the numbers are in the commit message.
> 
> Tested on a BC-250 running 7.2.6. Before:
> 
>    01:00.2 Encryption controller: AMD Device 143e
>            Memory at fe700000 [disabled] [size=1M]
>            Memory at fe884000 [disabled] [size=8K]
>            (no driver)
> 
> After:
> 
>    ccp 0000:01:00.2: enabling device (0000 -> 0002)
>    ccp 0000:01:00.2: platform access enabled
>    ccp 0000:01:00.2: psp enabled
>    # cat /sys/bus/pci/devices/0000:01:00.2/bootloader_version
>    00.1c.01.02
> 
> with the platform mailbox answering commands. Dynamic boost control is probed
> and cleanly rejected by this firmware, and HSTI reports nothing because the
> security reporting capability bit is clear. There is no CCP crypto engine
> behind this function: the version register at 0x100 reads back all ones.
> 
> The series is against v7.2.6 and touches only drivers/crypto/ccp/.
> 
> Mattia Tadini (3):
>    crypto: ccp - fix NULL dereference in psp_firmware_is_visible()
>    crypto: ccp - do not start PSP sub-devices without their vdata
>    crypto: ccp - add support for the AMD BC-250 secure processor
> 
>   drivers/crypto/ccp/psp-dev.c |  8 ++++++--
>   drivers/crypto/ccp/sp-pci.c  | 24 +++++++++++++++++++++++-
>   2 files changed, 29 insertions(+), 3 deletions(-)
> 

Sorry; but what's the point of adding support?  You can't access the 
crypto engine, it doesn't run SEV or TEE, it doesn't support DBC, it 
doesn't report HSTI.

It seems that the capability register isn't even populated on this system.

To me it appears the patch series is a lot of "fixes" to let you 
read.... the bootloader version.  Am I missing something else?

Re: [PATCH 0/3] crypto: ccp - two PSP init fixes, and the AMD BC-250

[Mattia Tadini]

On 9/21/26 10:10, Mario Limonciello wrote:
> Sorry; but what's the point of adding support?  You can't access the
> crypto engine, it doesn't run SEV or TEE, it doesn't support DBC, it
> doesn't report HSTI.

Thanks for looking at it. One correction first, and the mistake is
mine: the cover letter is wrong about HSTI. The security reporting bit
is clear, but psp_populate_hsti() then asks through the platform access
mailbox, and on this board PSP_CMD_HSTI_QUERY does answer. With the
series applied on 7.2.6:

  fused_part=1               debug_lock_on=1
  boot_integrity=0           tsme_status=0
  anti_rollback_status=0     rom_armor_enforced=0
  rpmc_production_enabled=0  rpmc_spirom_available=0
  hsp_tpm_available=0

> It seems that the capability register isn't even populated on this
> system.

It reads 0x00000002, the TEE bit and nothing else. That lone bit, with
no TEE behind it, is what patches 1 and 2 are about.

> To me it appears the patch series is a lot of "fixes" to let you
> read.... the bootloader version.  Am I missing something else?

Only the above. Once the device is bound, userspace gets the bootloader
version (fwupd picks it up as "Secure Processor", bootloader
00.1c.01.02) and the HSTI attributes. Nothing more: no crypto engine,
no SEV, no TEE, and the firmware rejects the DBC command.

If that is not enough to carry an ID, I understand, and patch 3 can go.
Patches 1 and 2 then have no reason to go in either: with the current
table I don't know of a shipped part that sets the TEE bit on a pspv1
or pspv2 function, so they would only guard against a device that is
not there.

So: would you take the series with the HSTI data as the justification
for patch 3 (I would send a v2 with a corrected cover letter), or
should I drop it?

Thanks,
Mattia

Re: [PATCH 0/3] crypto: ccp - two PSP init fixes, and the AMD BC-250

[Mario Limonciello]

On 9/22/26 05:05, Mattia Tadini wrote:
> On 9/21/26 10:10, Mario Limonciello wrote:
>> Sorry; but what's the point of adding support?  You can't access the
>> crypto engine, it doesn't run SEV or TEE, it doesn't support DBC, it
>> doesn't report HSTI.
> 
> Thanks for looking at it. One correction first, and the mistake is
> mine: the cover letter is wrong about HSTI. The security reporting bit
> is clear, but psp_populate_hsti() then asks through the platform access
> mailbox, and on this board PSP_CMD_HSTI_QUERY does answer. With the
> series applied on 7.2.6:
> 
>    fused_part=1               debug_lock_on=1
>    boot_integrity=0           tsme_status=0
>    anti_rollback_status=0     rom_armor_enforced=0
>    rpmc_production_enabled=0  rpmc_spirom_available=0
>    hsp_tpm_available=0
> 
>> It seems that the capability register isn't even populated on this
>> system.
> 
> It reads 0x00000002, the TEE bit and nothing else. That lone bit, with
> no TEE behind it, is what patches 1 and 2 are about.
> 
>> To me it appears the patch series is a lot of "fixes" to let you
>> read.... the bootloader version.  Am I missing something else?
> 
> Only the above. Once the device is bound, userspace gets the bootloader
> version (fwupd picks it up as "Secure Processor", bootloader
> 00.1c.01.02) and the HSTI attributes. Nothing more: no crypto engine,
> no SEV, no TEE, and the firmware rejects the DBC command.
> 
> If that is not enough to carry an ID, I understand, and patch 3 can go.
> Patches 1 and 2 then have no reason to go in either: with the current
> table I don't know of a shipped part that sets the TEE bit on a pspv1
> or pspv2 function, so they would only guard against a device that is
> not there.
> 
> So: would you take the series with the HSTI data as the justification
> for patch 3 (I would send a v2 with a corrected cover letter), or
> should I drop it?
> 
> Thanks,
> Mattia

It will be up to Tom here.

Given HSTI attributes do get exported from platform access mailbox that 
does change the shape.

But I do think that you should spin it to a v2 for the following reasons:

1) The cover letter is wrong (this discussion).

2) The first patch has a Fixes tag, but it's not really a bug until you 
add patch 3.  So it's in the right place in the series but I don't think 
it should have a Fixes tag.

3) I'm confused by your comments with TEE.

Why is the TEE capabilty set but TEE doesn't work?  Is there a problem 
with a guessed register layout or a real issue?

Rather than play whack a mole, wouldn't it be better to just clear 
psp->capability.tee when the ring init fails?  Then you can take pspv3 
layout.

4) If you DO end up sticking to a new register layout, you said up front 
in your cover letter DBC isn't supported.

Why do you set PLATFORM_FEATURE_DBC in your platform_features then in 
patch 3?  IMV this isn't going to be a relevant feature in the BC 250.

So I think that leaves two options for you to weigh out.

A) Either take the existing register pspv3 register layout and clear the 
TEE capability when the test fails
B) Take the new layout you proposed but don't advertise DBC feature.