linux-kernel - Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu

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: <CAKHBV24royJvLUHemST07i1G+xhEizO=VdTX0Q9itpi_dLv=4A@mail.gmail.com>
Date:   Tue, 15 Aug 2023 20:10:31 +0800
From:   Michael Shavit <mshavit@...gle.com>
To:     iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Cc:     will@...nel.org, robin.murphy@....com, nicolinc@...dia.com,
        jgg@...dia.com, jean-philippe@...aro.org
Subject: Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master

On Wed, Aug 9, 2023 at 1:15 AM Michael Shavit <mshavit@...gle.com> wrote:
>

> -static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> +static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
>                                        struct arm_smmu_master *master,
>                                        struct io_pgtable_cfg *pgtbl_cfg)
>  {
> @@ -2115,10 +2110,6 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>         if (ret)
>                 goto out_unlock;
>
> -       ret = arm_smmu_alloc_cd_tables(smmu_domain, master);
> -       if (ret)
> -               goto out_free_asid;
> -
>         cd->asid        = (u16)asid;
>         cd->ttbr        = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
>         cd->tcr         = FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
> @@ -2130,17 +2121,9 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>                           CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
>         cd->mair        = pgtbl_cfg->arm_lpae_s1_cfg.mair;
>
> -       ret = arm_smmu_write_ctx_desc(master, 0, cd);
> -       if (ret)
> -               goto out_free_cd_tables;
> -
>         mutex_unlock(&arm_smmu_asid_lock);
>         return 0;
>
> -out_free_cd_tables:
> -       arm_smmu_free_cd_tables(smmu_domain);
> -out_free_asid:
> -       arm_smmu_free_asid(cd);
>  out_unlock:
>         mutex_unlock(&arm_smmu_asid_lock);
>         return ret;
...
> @@ -2465,6 +2450,22 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>                 master->ats_enabled = arm_smmu_ats_supported(master);
>
> +       if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +               if (!master->cd_table.cdtab) {
> +                       ret = arm_smmu_alloc_cd_tables(master);
> +                       if (ret) {
> +                               master->domain = NULL;
> +                               return ret;
> +                       }
> +               }
> +
> +               ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd);
> +               if (ret) {
> +                       master->domain = NULL;
> +                       return ret;
> +               }
> +       }
> +
>         arm_smmu_install_ste_for_dev(master);
>
>         spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> @@ -2472,10 +2473,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
>         arm_smmu_enable_ats(master);

All this talk of locking on the other thread made me realize there's
an issue here. We are no longer holding the arm_smmu_asid_lock while
arm_smmu_write_ctx_desc is called due to its move out of
arm_smmu_domain_finalise_s1. This can race with arm_smmu_share_asid
which may modify the asid after we've written it, but before we've
updated the CD's smmu_domain->devices list.