| 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
| ||
|
Date: Wed, 14 Feb 2024 11:07:09 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Christophe Kerello <christophe.kerello@...s.st.com>,
miquel.raynal@...tlin.com, richard@....at, vigneshr@...com,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org
Cc: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com, devicetree@...r.kernel.org
Subject: Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support
On 13/02/2024 14:15, Christophe Kerello wrote:
>>> +
>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> + return 0;
>>> +
>>> + if (resource >= FMC2_MAX_RESOURCES)
>>> + return -EINVAL;
>>> +
>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
>
> Hi Krzysztof,
>
>>
>> No checking of read value?
>>
>
> No, it should never failed.
And you tested that neither smatch, sparse nor Coverity report here
warnings?
>
>>> + if (seccfgr & BIT(resource)) {
>>
>> Then on read failure this is random stack junk.
>>
>>> + if (resource)
>>> + dev_err(ebi->dev, "resource %d is configured as secure\n",
>>> + resource);
>>> +
>>> + return -EACCES;
>>> + }
>>> +
>>> + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
>>> + if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
>>> + /* CID filtering is turned off: access granted */
>>> + return 0;
>>> +
>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
>>> + /* Static CID mode */
>>> + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
>>> + if (cid != FMC2_CID1) {
>>> + if (resource)
>>> + dev_err(ebi->dev, "static CID%d set for resource %d\n",
>>> + cid, resource);
>>> +
>>> + return -EACCES;
>>> + }
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + /* Pass-list with semaphore mode */
>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
>>> + if (resource)
>>> + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
>>> + resource);
>>> +
>>> + return -EACCES;
>>> + }
>>> +
>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>> + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>> + }
>>> +
>>> + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
>>> + if (cid != FMC2_CID1) {
>>> + if (resource)
>>> + dev_err(ebi->dev, "resource %d is already used by CID%d\n",
>>> + resource, cid);
>>> +
>>> + return -EACCES;
>>> + }
>>> +
>>> + ebi->sem_taken |= BIT(resource);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
>>> +{
>>> + unsigned int resource;
>>> +
>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> + return;
>>> +
>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>> + if (!(ebi->sem_taken & BIT(resource)))
>>> + continue;
>>> +
>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> + FMC2_SEMCR_SEM_MUTEX, 0);
>>> + }
>>> +}
>>> +
>>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
>>> +{
>>> + unsigned int resource;
>>> +
>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> + return;
>>> +
>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>> + if (!(ebi->sem_taken & BIT(resource)))
>>> + continue;
>>> +
>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>> + }
>>> +}
>>> +
>>> static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
>>> struct device_node *dev_node,
>>> const struct stm32_fmc2_prop *prop,
>>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>> unsigned int cs;
>>>
>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>> + if (!(ebi->bank_assigned & BIT(cs)))
>>> + continue;
>>> +
>>> regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
>>> regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
>>> regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
>>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>>
>>> if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
>>> - else
>>> + else if (ebi->access_granted)
>>> regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
>>> }
>>>
>>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>> unsigned int cs;
>>>
>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>> + if (!(ebi->bank_assigned & BIT(cs)))
>>> + continue;
>>> +
>>> regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
>>> regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
>>> regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
>>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>>
>>> if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
>>> - else
>>> + else if (ebi->access_granted)
>>> regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
>>
>> So this is kind of half-allowed-half-not. How is it supposed to work
>> with !access_granted? You configure some registers but some not. So will
>> it work or not? If yes, why even needing to write to FMC2_CFGR!
>>
>
> This register is considered as one resource and can be protected. If a
> companion (like optee_os) has configured this resource as secure, it
> means that the driver can not write into this register, and this
> register will be handled by the companion. If this register is let as
> non secure, the driver can handle this ressource.
So this is not an error? Other places print errors and return -EACCESS,
so that's a bit confusing me.
Best regards,
Krzysztof
Powered by blists - more mailing lists