linux-kernel - Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Match Stall behaviour for S2

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZtH8Yj/VBHrzioSH@Asurada-Nvidia>
Date: Fri, 30 Aug 2024 10:07:46 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Robin Murphy <robin.murphy@....com>
CC: Mostafa Saleh <smostafa@...gle.com>, <linux-kernel@...r.kernel.org>,
	<iommu@...ts.linux.dev>, <linux-arm-kernel@...ts.infradead.org>,
	<will@...nel.org>, <joro@...tes.org>, <jean-philippe@...aro.org>,
	<jgg@...pe.ca>, <mshavit@...gle.com>
Subject: Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Match Stall behaviour for S2

On Fri, Aug 30, 2024 at 06:02:35PM +0100, Robin Murphy wrote:

> On 30/08/2024 5:30 pm, Nicolin Chen wrote:
> > On Fri, Aug 30, 2024 at 11:03:47AM +0000, Mostafa Saleh wrote:
> > 
> > > According to the spec (ARM IHI 0070 F.b), in
> > > "5.5 Fault configuration (A, R, S bits)":
> > >      A STE with stage 2 translation enabled and STE.S2S == 0 is
> > >      considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10.
> > > 
> > > Also described in the pseudocode “SteIllegal()”
> > >      if STE.Config == '11x' then
> > >          [..]
> > >          if eff_idr0_stall_model == '10' && STE.S2S == '0' then
> > >              // stall_model forcing stall, but S2S == 0
> > >              return TRUE;
> > > 
> > > Which means, S2S must be set when stall model is
> > > "ARM_SMMU_FEAT_STALL_FORCE", but currently the driver ignores that.
> > > 
> > > Although, the driver can do the minimum and only set S2S for
> > > “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1
> > > behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the
> > > master has requested stalls.
> > 
> > If I read the SteIllegal() correctly, it seems S2S would conflict
> > against the STE.EATS settings?
> > 
> > // Check ATS configuration
> > if ((sec_sid == SS_NonSecure && SMMU_IDR0.ATS == '1') ||
> >      (sec_sid == SS_Realm && SMMU_R_IDR0.ATS == '1')) &&
> >      STE.Config != 'x00' then
> >      // Needs to be NS/Realm, ATS enabled, and not Bypass
> >          if STE.EATS == '01' && STE.S2S == '1' then
> >              // Full ATS mode
> >              if STE.Config == '11x' || constr_unpred_EATS_S2S then
> >                  // if stage 2 enabled or CONSTRAINED UNPREDICTABLE for SMMUv3.0
> >                  return TRUE;
> > 
> > So, if master->stall_enabled and master->ats_enabled, there would
> > be a bad STE?
> 
> Indeed, but as discussed previously, to get there would require either
> firmware or hardware to bogusly advertise both stall and ATS
> capabilities for the same device, which we decided is beyond the scope
> of what's worth trying to reason about. If a nonsensical system leads to
> obviously blowing up with C_BAD_STE, that's arguably not such a bad thing.

Oh, I see. Thanks for the note!

Nicolin