| 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: <3962320b58beaa4626ed69b3120d4246@codeaurora.org>
Date: Thu, 04 Mar 2021 20:46:35 +0530
From: pragalla@...eaurora.org
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Pradeep P V K <pragalla@....qualcomm.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Asutosh Das <asutoshd@...eaurora.org>,
Sahitya Tummala <stummala@...eaurora.org>,
Ram Prakash Gupta <rampraka@...eaurora.org>,
Veerabhadrarao Badiganti <vbadigan@...eaurora.org>,
linux-mmc@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap
On 2021-03-04 19:19, Ulf Hansson wrote:
> On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <pragalla@....qualcomm.com>
> wrote:
>>
>> From: Pradeep P V K <pragalla@...eaurora.org>
>>
>> For data read commands, SDHC may initiate data transfers even before
>> it
>> completely process the command response. In case command itself fails,
>> driver un-maps the memory associated with data transfer but this
>> memory
>> can still be accessed by SDHC for the already initiated data transfer.
>> This scenario can lead to un-mapped memory access error.
>>
>> To avoid this scenario, reset SDHC (when command fails) prior to
>> un-mapping memory. Resetting SDHC ensures that all in-flight data
>> transfers are either aborted or completed. So we don't run into this
>> scenario.
>>
>> Swap the reset, un-map steps sequence in sdhci_request_done().
>>
>> Changes since V1:
>> - Added an empty line and fixed the comment style.
>> - Retained the Acked-by signoff.
>>
>> Suggested-by: Veerabhadrarao Badiganti <vbadigan@...eaurora.org>
>> Signed-off-by: Pradeep P V K <pragalla@...eaurora.org>
>> Acked-by: Adrian Hunter <adrian.hunter@...el.com>
Hi Uffe,
>
> Seems like it might be a good idea to tag this for stable? I did that,
> but awaiting for your confirmation.
>
Yes, this fix is applicable for all stable starting from 4.9 (n/a for
4.4).
Kindly go ahead.
> So, applied for next, thanks!
>
> Kind regards
> Uffe
>
Thanks and Regards,
Pradeep
>
>> ---
>> drivers/mmc/host/sdhci.c | 60
>> +++++++++++++++++++++++++-----------------------
>> 1 file changed, 31 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 646823d..130fd2d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2998,6 +2998,37 @@ static bool sdhci_request_done(struct
>> sdhci_host *host)
>> }
>>
>> /*
>> + * The controller needs a reset of internal state machines
>> + * upon error conditions.
>> + */
>> + if (sdhci_needs_reset(host, mrq)) {
>> + /*
>> + * Do not finish until command and data lines are
>> available for
>> + * reset. Note there can only be one other mrq, so it
>> cannot
>> + * also be in mrqs_done, otherwise host->cmd and
>> host->data_cmd
>> + * would both be null.
>> + */
>> + if (host->cmd || host->data_cmd) {
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return true;
>> + }
>> +
>> + /* Some controllers need this kick or reset won't work
>> here */
>> + if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
>> + /* This is to force an update */
>> + host->ops->set_clock(host, host->clock);
>> +
>> + /*
>> + * Spec says we should do both at the same time, but
>> Ricoh
>> + * controllers do not like that.
>> + */
>> + sdhci_do_reset(host, SDHCI_RESET_CMD);
>> + sdhci_do_reset(host, SDHCI_RESET_DATA);
>> +
>> + host->pending_reset = false;
>> + }
>> +
>> + /*
>> * Always unmap the data buffers if they were mapped by
>> * sdhci_prepare_data() whenever we finish with a request.
>> * This avoids leaking DMA mappings on error.
>> @@ -3060,35 +3091,6 @@ static bool sdhci_request_done(struct
>> sdhci_host *host)
>> }
>> }
>>
>> - /*
>> - * The controller needs a reset of internal state machines
>> - * upon error conditions.
>> - */
>> - if (sdhci_needs_reset(host, mrq)) {
>> - /*
>> - * Do not finish until command and data lines are
>> available for
>> - * reset. Note there can only be one other mrq, so it
>> cannot
>> - * also be in mrqs_done, otherwise host->cmd and
>> host->data_cmd
>> - * would both be null.
>> - */
>> - if (host->cmd || host->data_cmd) {
>> - spin_unlock_irqrestore(&host->lock, flags);
>> - return true;
>> - }
>> -
>> - /* Some controllers need this kick or reset won't work
>> here */
>> - if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
>> - /* This is to force an update */
>> - host->ops->set_clock(host, host->clock);
>> -
>> - /* Spec says we should do both at the same time, but
>> Ricoh
>> - controllers do not like that. */
>> - sdhci_do_reset(host, SDHCI_RESET_CMD);
>> - sdhci_do_reset(host, SDHCI_RESET_DATA);
>> -
>> - host->pending_reset = false;
>> - }
>> -
>> host->mrqs_done[i] = NULL;
>>
>> spin_unlock_irqrestore(&host->lock, flags);
>> --
>> 2.7.4
>>
Powered by blists - more mailing lists