Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Generic PAS TEE backend maps TA-level errors to -EINVAL, breaking the DRM MSM fallback logic. - [High] TOCTOU race condition in generic PAS API wrappers leading to NULL pointer dereference. - [High] Unconditional PAS availability check causes infinite probe deferral on platforms without PAS support. Pre-existing issues: - [High] The MSM DRM driver propagates -EOPNOTSUPP instead of -ENODEV, fatally failing GPU initialization. -- commit fd17ff54a9520d92288105f263b3bc29eb9cbd4b Author: Sumit Garg <[email protected]> drm/msm: Switch to generic PAS TZ APIs Switch drm/msm client drivers over to generic PAS TZ APIs. Generic PAS TZ service allows to support multiple TZ implementation backends like QTEE based SCM PAS service, OP-TEE based PAS service and any further future TZ backend service. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index c62c45bb0ddbb..489462065ea95 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c [ ... ] > @@ -146,10 +147,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, > const char *fwname, > goto out; > > /* Send the image to the secure world */ > - ret = qcom_scm_pas_auth_and_reset(pasid); > + ret = qcom_pas_auth_and_reset(pasid); [Severity: High] Does this new PAS API wrapper have a TOCTOU race condition? If the PAS backend unregisters concurrently, could ops_ptr become NULL between the check and the dereference in qcom_pas_auth_and_reset()? drivers/firmware/qcom/qcom_pas.c:qcom_pas_auth_and_reset() { if (!ops_ptr) return -ENODEV; return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id); } > > /* > - * If the scm call returns -EOPNOTSUPP we assume that this target > + * If the pas call returns -EOPNOTSUPP we assume that this target > * doesn't need/support the zap shader so quietly fail > */ > if (ret == -EOPNOTSUPP) [Severity: High] Is the DRM MSM fallback logic preserved with this generic backend? The driver expects -EOPNOTSUPP to gracefully handle unsupported zap shaders. If the generic PAS TEE backend's __qcom_pas_tee_auth_and_reset() unconditionally maps all TA errors to -EINVAL, will this cause GPU probe to fail fatally on TEE platforms where the zap shader is not supported? drivers/firmware/qcom/qcom_pas_tee.c:__qcom_pas_tee_auth_and_reset() { ret = tee_client_invoke_func(data->ctx, &inv_arg, param); if (ret < 0 || inv_arg.ret != 0) { ... return ret ?: -EINVAL; } } [Severity: High] This isn't a bug introduced by this patch, but does this function propagate -EOPNOTSUPP instead of -ENODEV? If qcom_pas_auth_and_reset() returns -EOPNOTSUPP, the code sets zap_available = false but leaves ret = -EOPNOTSUPP. Since callers explicitly check for -ENODEV to fall back to SECVID_TRUST_CNTL, could this unmapped -EOPNOTSUPP abort hardware initialization? > zap_available = false; > else if (ret) > DRM_DEV_ERROR(dev, "Unable to authorize the image\n"); [ ... ] > @@ -175,9 +176,9 @@ int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid) > if (!zap_available) > return -ENODEV; > > - /* We need SCM to be able to load the firmware */ > - if (!qcom_scm_is_available()) { > - DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n"); > + /* We need PAS to be able to load the firmware */ > + if (!qcom_pas_is_available()) { > + DRM_DEV_ERROR(&pdev->dev, "PAS is not available\n"); > return -EPROBE_DEFER; > } [Severity: High] Could this unconditional PAS availability check cause infinite probe deferral on platforms without PAS support? By checking qcom_pas_is_available() before checking if the zap-shader DT node exists (which happens inside zap_shader_load_mdt()), do platforms that lack PAS capabilities fail to reach the DT node check and defer indefinitely? Previously, qcom_scm_is_available() returned true on these platforms, allowing the driver to gracefully return -ENODEV when the DT node was missing. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
