| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <505e7b7c-11dc-b932-09b0-93b47b031750@amd.com>
Date: Wed, 8 Feb 2023 11:23:20 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org,
Brijesh Singh <brijesh.singh@....com>,
"Kalra, Ashish" <ashish.kalra@....com>,
linux-crypto@...r.kernel.org
Subject: Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device
On 2/8/23 06:45, Jeremi Piotrowski wrote:
> On 06/02/2023 20:13, Tom Lendacky wrote:
>> On 2/1/23 13:24, Jeremi Piotrowski wrote:
>>> On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote:
>>>> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>>>>> When matching the "psp" platform_device, determine the register offsets
>>>>> at runtime from the ASP ACPI table. Pass the parsed register offsets
>>>> >from the ASPT through platdata.
>>>>>
>>>>> To support this scenario, mark the members of 'struct sev_vdata' and
>>>>> 'struct psp_vdata' non-const so that the probe function can write the
>>>>> values. This does not affect the other users of sev_vdata/psp_vdata as
>>>>> they define the whole struct const and the pointer in struct
>>>>> sp_dev_vdata stays const too.
>>>>>
>>>>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
>>>>> ---
>>>>> arch/x86/kernel/psp.c | 3 ++
>>>>> drivers/crypto/ccp/sp-dev.h | 12 +++----
>>>>> drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
>>>>> 3 files changed, 65 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
>>>>> index 24181d132bae..68511a14df63 100644
>>>>> --- a/arch/x86/kernel/psp.c
>>>>> +++ b/arch/x86/kernel/psp.c
>>>>> @@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
>>>>> if (err)
>>>>> return err;
>>>>> err = platform_device_add_resources(&psp_device, res, 2);
>>>>> + if (err)
>>>>> + return err;
>>>>> + err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
>>>>> if (err)
>>>>> return err;
>>>>> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
>>>>> index 20377e67f65d..aaa651364425 100644
>>>>> --- a/drivers/crypto/ccp/sp-dev.h
>>>>> +++ b/drivers/crypto/ccp/sp-dev.h
>>>>> @@ -40,9 +40,9 @@ struct ccp_vdata {
>>>>> };
>>>>> struct sev_vdata {
>>>>> - const unsigned int cmdresp_reg;
>>>>> - const unsigned int cmdbuff_addr_lo_reg;
>>>>> - const unsigned int cmdbuff_addr_hi_reg;
>>>>> + unsigned int cmdresp_reg;
>>>>> + unsigned int cmdbuff_addr_lo_reg;
>>>>> + unsigned int cmdbuff_addr_hi_reg;
>>>>> };
>>>>> struct tee_vdata {
>>>>> @@ -56,9 +56,9 @@ struct tee_vdata {
>>>>> struct psp_vdata {
>>>>> const struct sev_vdata *sev;
>>>>> const struct tee_vdata *tee;
>>>>> - const unsigned int feature_reg;
>>>>> - const unsigned int inten_reg;
>>>>> - const unsigned int intsts_reg;
>>>>> + unsigned int feature_reg;
>>>>> + unsigned int inten_reg;
>>>>> + unsigned int intsts_reg;
>>>>> };
>>>>> /* Structure to hold SP device data */
>>>>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>>>>> index ea8926e87981..281dbf6b150c 100644
>>>>> --- a/drivers/crypto/ccp/sp-platform.c
>>>>> +++ b/drivers/crypto/ccp/sp-platform.c
>>>>> @@ -22,6 +22,7 @@
>>>>> #include <linux/of.h>
>>>>> #include <linux/of_address.h>
>>>>> #include <linux/acpi.h>
>>>>> +#include <linux/platform_data/psp.h>
>>>>> #include "ccp-dev.h"
>>>>> @@ -30,11 +31,31 @@ struct sp_platform {
>>>>> unsigned int irq_count;
>>>>> };
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>> +static struct sev_vdata sev_platform = {
>>>>> + .cmdresp_reg = -1,
>>>>> + .cmdbuff_addr_lo_reg = -1,
>>>>> + .cmdbuff_addr_hi_reg = -1,
>>>>> +};
>>>>> +static struct psp_vdata psp_platform = {
>>>>> + .sev = &sev_platform,
>>>>> + .feature_reg = -1,
>>>>> + .inten_reg = -1,
>>>>> + .intsts_reg = -1,
>>>>> +};
>>>>> +#endif
>>>>> +
>>>>> static const struct sp_dev_vdata dev_vdata[] = {
>>>>> {
>>>>> .bar = 0,
>>>>> #ifdef CONFIG_CRYPTO_DEV_SP_CCP
>>>>> .ccp_vdata = &ccpv3_platform,
>>>>> +#endif
>>>>> + },
>>>>> + {
>>>>> + .bar = 0,
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>> + .psp_vdata = &psp_platform,
>>>>> #endif
>>>>> },
>>>>> };
>>>>> @@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
>>>>> #endif
>>>>> static const struct platform_device_id sp_plat_match[] = {
>>>>> - { "psp" },
>>>>> + { "psp", (kernel_ulong_t)&dev_vdata[1] },
>>>>> { },
>>>>> };
>>>>> MODULE_DEVICE_TABLE(platform, sp_plat_match);
>>>>> @@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
>>>>> return NULL;
>>>>> }
>>>>> +static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
>>>>> +{
>>>>> + struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
>>>>
>>>> s/drvdata/vdata/
>>>>
>>>
>>> ok
>>>
>>>>> + struct device *dev = &pdev->dev;
>>>>> +
>>>>
>>>> Should check for null vdata and return NULL, e.g.:
>>>>
>>>> if (!vdata)
>>>> return NULL;
>>>>
>>>
>>> ok
>>>
>>>>> + if (drvdata == &dev_vdata[1]) {
>>>>
>>>> This should be a check for vdata->psp_vdata being non-NULL and
>>>> vdata->psp_vdata->sev being non-NULL, e.g.:
>>>>
>>>> if (vdata->psp_vdata && vdata->psp_vdata->sev) {
>>>>
>>>>> + struct psp_platform_data *pdata = dev_get_platdata(dev);
>>>>> +
>>>>> + if (!pdata) {
>>>>> + dev_err(dev, "missing platform data\n");
>>>>> + return NULL;
>>>>> + }
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>
>>>> No need for this with the above checks
>>>>
>>>>> + psp_platform.feature_reg = pdata->feature_reg;
>>>>
>>>> These should then be:
>>>>
>>>> vdata->psp_vdata->inten_reg = pdata->feature_reg;
>>>> ...
>>>
>>> I see where you're going with this and the above suggestions, but
>>> the psp_vdata pointer is const in struct sp_dev_vdata and so is the
>>> sev pointer in struct psp_vdata. I find these consts to be important
>>> and doing it this way would require casting away the const. I don't
>>> think that's worth doing.
>>
>> Ok, then maybe it would be better to kmalloc a vdata structure that you fill in and then assign that to dev_vdata field for use. That could eliminate the removal of the "const" notations in one of the previous patches. I just don't think you should be changing the underlying module data that isn't expected to be changed.
>>
>
> I can do that and undo the removal of consts from the
> struct (sev|psp)_vdata members, but the outcome would look something
> like this:
>
> static void sp_platform_fill_vdata(struct sp_dev_vdata *vdata,
> struct psp_vdata *psp,
> struct sev_vdata *sev,
> const struct psp_platform_data *pdata)
> {
> struct sev_vdata sevtmp = {
> .cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg,
> .cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg,
> .cmdresp_reg = pdata->sev_cmd_resp_reg,
> };
> struct psp_vdata psptmp = {
> .feature_reg = pdata->feature_reg,
> .inten_reg = pdata->irq_en_reg,
> .intsts_reg = pdata->irq_st_reg,
> .sev = sev,
> };
>
> memcpy(sev, &sevtmp, sizeof(*sev));
> memcpy(psp, &psptmp, sizeof(*psp));
> vdata->psp_vdata = psp;
> }
>
> static struct sp_dev_vdata *sp_get_platform_version(struct sp_device *sp)
> {
> struct sp_platform *sp_platform = sp->dev_specific;
> struct psp_platform_data *pdata;
> struct device *dev = sp->dev;
> struct sp_dev_vdata *vdata;
> struct psp_vdata *psp;
> struct sev_vdata *sev;
>
> pdata = dev_get_platdata(dev);
> if (!pdata) {
> dev_err(dev, "missing platform data\n");
> return NULL;
> }
>
> sp_platform->is_platform_device = true;
>
> vdata = devm_kzalloc(dev, sizeof(*vdata) + sizeof(*psp) + sizeof(*sev), GFP_KERNEL);
> if (!vdata)
> return NULL;
>
> psp = (void *)vdata + sizeof(*vdata);
> sev = (void *)psp + sizeof(*psp);
> sp_platform_fill_vdata(vdata, psp, sev, pdata);
>
> /* elided debug print */
> ...
>
> return vdata;
> }
>
> with the const fields in the struct it's not possible to assign in any
> other way than on initialization, so I need to use the helper function,
> tmp structs and memcpy.
Yeah, not the prettiest, but I prefer this over altering the static data.
>
> Could you ack that you like this approach before I post a v2?
Yes, please go with this approach.
Thanks,
Tom
Powered by blists - more mailing lists