Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] NULL pointer dereference in fastrpc_rpmsg_probe() when allocating memory for SDSP_DOMAIN_ID on systems without the qcom,vmids property. - [High] SMP initialization is unconditionally disabled on Qualcomm ARM32 platforms. - [Low] Unupdated caller of `qcom_scm_io_writel()` in the USB EUD driver. - [Low] Unupdated caller of `qcom_scm_mem_protect_video_var()` in the Iris media driver. --
commit 9df6c4c915fd9c1b7019af817a1a3e4285555806 Author: Albert Esteve <[email protected]> firmware: qcom: scm: pass qcom_scm through the exported API This commit modifies the exported SCM calls to take an explicit struct qcom_scm handle instead of using a global singleton. Subsystems and device drivers are updated to recover the instance via dev_get_drvdata() or qcom_scm_get() before calling SCM APIs. > diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c > index eca2fe0f4314b..c4e7aab77bc99 100644 > --- a/arch/arm/mach-qcom/platsmp.c > +++ b/arch/arm/mach-qcom/platsmp.c > @@ -355,9 +355,10 @@ static int kpssv2_boot_secondary(unsigned int cpu, > struct task_struct *idle) > > static void __init qcom_smp_prepare_cpus(unsigned int max_cpus) > { > + struct qcom_scm *scm = qcom_scm_get(); > int cpu; > > - if (qcom_scm_set_cold_boot_addr(secondary_startup_arm)) { > + if (!scm || qcom_scm_set_cold_boot_addr(scm, secondary_startup_arm)) { [Severity: High] Does this code unintentionally disable SMP on all Qualcomm ARM32 devices? Because qcom_smp_prepare_cpus() is called early in the boot process (from smp_prepare_cpus()), before do_initcalls() probes the qcom_scm platform driver, qcom_scm_get() will unconditionally return NULL here. This causes the (!scm) condition to short-circuit to true, skipping the SCM call and executing the error path which clears the cpu_present mask for all secondary CPUs. [ ... ] > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 90fd669636ec1..e8ff9d4b03957 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2569,13 +2574,17 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > "qcom,vmids", &vmids[0], 0, FASTRPC_MAX_VMIDS); > if (vmcount < 0) > vmcount = 0; > - else if (!qcom_scm_is_available()) > - return -EPROBE_DEFER; > + else { > + scm = qcom_scm_get(); > + if (!scm) > + return -EPROBE_DEFER; > + } > > data = kzalloc_obj(*data); > if (!data) > return -ENOMEM; > > + data->scm = scm; > if (vmcount) { > data->vmcount = vmcount; > for (i = 0; i < data->vmcount; i++) { > @@ -2592,7 +2601,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > if (!err) { > src_perms = BIT(QCOM_SCM_VMID_HLOS); > > - err = qcom_scm_assign_mem(res.start, > resource_size(&res), &src_perms, > + err = qcom_scm_assign_mem(scm, res.start, > resource_size(&res), &src_perms, [Severity: High] Can this result in a NULL pointer dereference in qcom_scm_assign_mem() when allocating TZ memory? If the "qcom,vmids" DT property is missing, vmcount defaults to 0 and the "else" block above is skipped, leaving the scm pointer as NULL. Later in fastrpc_rpmsg_probe(), for the SDSP_DOMAIN_ID domain with a reserved memory region, this code unconditionally passes the NULL scm pointer to qcom_scm_assign_mem(). Inside qcom_scm_assign_mem(), the NULL pointer is immediately dereferenced during the memory allocation call: qcom_tzmem_alloc(scm->mempool, ...) [ ... ] > diff --git a/include/linux/firmware/qcom/qcom_scm.h > b/include/linux/firmware/qcom/qcom_scm.h > index 5747bd191bf15..9c513fe6f0ea9 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h [ ... ] > -int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); > -int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); > +int qcom_scm_io_readl(struct qcom_scm *scm, phys_addr_t addr, unsigned int > *val); > +int qcom_scm_io_writel(struct qcom_scm *scm, phys_addr_t addr, unsigned int > val); [Severity: Low] Are there unupdated callers of qcom_scm_io_writel() that will cause a build failure? The enable_eud() and disable_eud() functions in drivers/usb/misc/qcom_eud.c appear to call qcom_scm_io_writel() without the newly required struct qcom_scm pointer as the first argument. [ ... ] > -int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size, > +int qcom_scm_mem_protect_video_var(struct qcom_scm *scm, u32 cp_start, u32 > cp_size, > u32 cp_nonpixel_start, u32 cp_nonpixel_size); [Severity: Low] Could this signature change also cause a build failure? The iris_set_cp_prot() function in drivers/media/platform/qcom/iris/iris_firmware.c appears to still call qcom_scm_mem_protect_video_var() without the newly required struct qcom_scm pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
