linux-kernel - Re: [PATCH v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

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: <3jrz5dtblgmacp32zda6yai76qkp3wxzj5axj7cwnzpdgk3uxr@5tnwyayvzlyu>
Date: Thu, 26 Jun 2025 14:09:08 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Johan Hovold <johan@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, Ard Biesheuvel <ardb@...nel.org>,
        Steev Klimaszewski <steev@...i.org>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-efi@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

On Thu, Jun 26, 2025 at 11:56:01AM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:53:26AM +0300, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > 
> > Listing individual machines in qcom_scm_qseecom_allowlist doesn't scale.
> > Allow it to function as allow and disallow list at the same time by the
> > means of the match->data and list the SoC families instead of devices.
> > 
> > In case a particular device has buggy or incompatible firmware user
> > still can disable QSEECOM by specifying qcom_scm.qseecom=off kernel
> > param and (in the longer term) adding machine-specific entry to the
> > qcom_scm_qseecom_allowlist table.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> 
> >  /*
> >   * We do not yet support re-entrant calls via the qseecom interface. To prevent
> > - * any potential issues with this, only allow validated machines for now. Users
> > + * any potential issues with this, only allow validated platforms for now. Users
> >   * still can manually enable or disable it via the qcom_scm.qseecom modparam.
> > + *
> > + * To disable QSEECOM for a particular machine, add compatible entry and set
> > + * data to &qcom_qseecom_disable.
> >   */
> >  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> > -	{ .compatible = "asus,vivobook-s15" },
> > -	{ .compatible = "asus,zenbook-a14-ux3407qa" },
> > -	{ .compatible = "asus,zenbook-a14-ux3407ra" },
> > -	{ .compatible = "dell,xps13-9345" },
> > -	{ .compatible = "hp,elitebook-ultra-g1q" },
> > -	{ .compatible = "hp,omnibook-x14" },
> > -	{ .compatible = "huawei,gaokun3" },
> > -	{ .compatible = "lenovo,flex-5g" },
> > -	{ .compatible = "lenovo,thinkpad-t14s" },
> > -	{ .compatible = "lenovo,thinkpad-x13s", },
> >  	{ .compatible = "lenovo,yoga-c630", .data = &qcom_qseecom_ro_uefi, },
> > -	{ .compatible = "lenovo,yoga-slim7x" },
> > -	{ .compatible = "microsoft,arcata", },
> > -	{ .compatible = "microsoft,blackrock" },
> > -	{ .compatible = "microsoft,romulus13", },
> > -	{ .compatible = "microsoft,romulus15", },
> > -	{ .compatible = "qcom,sc8180x-primus" },
> > +	{ .compatible = "qcom,sc8180x", },
> > +	{ .compatible = "qcom,sc8280xp", },
> >  	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> 
> You need to have the machine specific entries before the SoC fallbacks
> for this to work.

I don't think so. It's not how OF matching works.

> 
> Perhaps this should be made more clear in the table by adding a
> separator comment before the SoC entries or similar.
> 
> > -	{ .compatible = "qcom,x1e001de-devkit" },
> > -	{ .compatible = "qcom,x1e80100-crd" },
> > -	{ .compatible = "qcom,x1e80100-qcp" },
> > -	{ .compatible = "qcom,x1p42100-crd" },
> > +	{ .compatible = "qcom,sdm845", .data = &qcom_qseecom_disable, },
> > +	{ .compatible = "qcom,x1e80100", },
> > +	{ .compatible = "qcom,x1p42100", },
> >  	{ }
> >  };
> >  
> > @@ -2046,12 +2035,22 @@ static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev,
> >  	match = of_match_node(qcom_scm_qseecom_allowlist, np);
> >  	of_node_put(np);
> >  
> > -	if (match && match->data)
> > +	if (!match) {
> > +		dev_info(scm_dev, "qseecom: untested machine, skipping\n");
> > +		return false;
> > +	}
> > +
> > +	if (match->data)
> >  		*quirks = *(unsigned long *)(match->data);
> >  	else
> >  		*quirks = 0;
> >  
> > -	return match;
> > +	if (*quirks & QCOM_QSEECOM_QUIRK_DISABLE) {
> > +		dev_info(scm_dev, "qseecom: disabled by the quirk\n");
> 
> Not sure this is needed since it presumably has been disabled because it
> has been tested and found not to work. No need to spam the logs with
> that on every boot.
> 
> In any case I don't think you should be referring to "the quirk" which
> makes little sense without looking at the implementation.

ack

> 
> > +		return false;
> > +	}
> > +
> > +	return true;
> >  }
> 
> Johan

-- 
With best wishes
Dmitry