linux-kernel - RE: [PATCH] iommu/arm-smmu-v3: fixed check process for disable

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <TYCPR01MB6160FF56DA7F9DEB2841C656E99C9@TYCPR01MB6160.jpnprd01.prod.outlook.com>
Date:   Wed, 3 Aug 2022 12:45:09 +0000
From:   "ishii.shuuichir@...itsu.com" <ishii.shuuichir@...itsu.com>
To:     'Robin Murphy' <robin.murphy@....com>,
        "will@...nel.org" <will@...nel.org>,
        "joro@...tes.org" <joro@...tes.org>,
        "thunder.leizhen@...wei.com" <thunder.leizhen@...wei.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "chenxiang66@...ilicon.com" <chenxiang66@...ilicon.com>,
        "christophe.jaillet@...adoo.fr" <christophe.jaillet@...adoo.fr>,
        "john.garry@...wei.com" <john.garry@...wei.com>,
        "baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "ishii.shuuichir@...itsu.com" <ishii.shuuichir@...itsu.com>
Subject: RE: [PATCH] iommu/arm-smmu-v3: fixed check process for disable_bypass
 module parameter

Hi, Robin,
Thank you for your comments.

> >   	/* Enable the SMMU interface, or ensure bypass */
> > -	if (!bypass || disable_bypass) {
> > +	if (!bypass && disable_bypass) {
> 
> This change looks obviously wrong - if bypass is false here then we definitely
> want to enable the SMMU, so disable_bypass is irrelevant. It shouldn't even be
> possible to get here with bypass==true under ACPI, since
> arm_smmu_device_acpi_probe() cannot fail :/

Sorry, my understanding of the meaning of the disable_bypass module parameter 
and the process of setting GBPA_ABORT was insufficient.

I misunderstood that the disable_bypass module parameter is used to simply 
bypass (disable) SMMU (SMMU_CR0.SMMUEN == 0 and SMMU_GBPA.ABORT == 0).
Forget about the fixes in this patch.

Although our understanding was lacking, 
we thought it would be a good idea to have a module parameter that simply disables SMMU,
so we were considering a fix.

Best regards, 
Shuuichirou.

> -----Original Message-----
> From: Robin Murphy <robin.murphy@....com>
> Sent: Wednesday, August 3, 2022 6:26 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@...itsu.com>;
> will@...nel.org; joro@...tes.org; thunder.leizhen@...wei.com; jgg@...pe.ca;
> tglx@...utronix.de; chenxiang66@...ilicon.com; christophe.jaillet@...adoo.fr;
> john.garry@...wei.com; baolu.lu@...ux.intel.com;
> linux-arm-kernel@...ts.infradead.org; iommu@...ts.linux.dev;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] iommu/arm-smmu-v3: fixed check process for
> disable_bypass module parameter
> 
> On 2022-08-03 00:42, Shuuichirou Ishii wrote:
> > The current process does not enable the bypass setting regardless of
> > the value of the disable_bypass module parameter when ACPI is enabled,
> > so the value of the disable_bypass module parameter has been corrected
> > so that it is handled correctly.
> >
> > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@...itsu.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 88817a3376ef..256d7b2a83a7 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3396,7 +3396,7 @@ static int arm_smmu_device_reset(struct
> arm_smmu_device *smmu, bool bypass)
> >   		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
> >
> >   	/* Enable the SMMU interface, or ensure bypass */
> > -	if (!bypass || disable_bypass) {
> > +	if (!bypass && disable_bypass) {
> 
> This change looks obviously wrong - if bypass is false here then we definitely
> want to enable the SMMU, so disable_bypass is irrelevant. It shouldn't even be
> possible to get here with bypass==true under ACPI, since
> arm_smmu_device_acpi_probe() cannot fail :/
> Robin.
> 
> >   		enables |= CR0_SMMUEN;
> >   	} else {
> >   		ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);