| 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: <3ed3234e-e811-71ef-41f9-d7999066b62d@linux.microsoft.com>
Date: Wed, 8 Feb 2023 13:45:10 +0100
From: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
To: Tom Lendacky <thomas.lendacky@....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 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.
Could you ack that you like this approach before I post a v2?
Powered by blists - more mailing lists