ACK: [SRU][J][PATCH v2 0/1] CVE-2024-26893

[Thibault Ferrante]

Acked-by: Thibault Ferrante <thibault.ferrante at canonical.com>


On 10-09-2024 10:48, Hui Wang wrote:
> In the v2: dropped the 0001-xxx.patch, this patch could help resolve
> context conflict when applying the CVE fixing patch, and it calls
> free_irq() to fix a irq triggering issue. But this issue is unrelevant
> to this CVE case and this patch could introduce build error, to fix
> the build error, need to backport at least 2 other patches which will
> change the smc transport to use common completion, this is a big
> change and could introduce regression. Hence drop the patch in the V2.
> 
> [Impact]
> 
> When the generic SCMI code tears down a channel, it calls the chan_free
> callback function, defined by each transport. Since multiple protocols
> might share the same transport_info member, chan_free() might want to
> clean up the same member multiple times within the given SCMI transport
> implementation. In this case, it is SMC transport. This will lead to a NULL
> pointer dereference at the second time.
> 
> 
> [Backport]
> 
> This backporting has 2 change compared to the original commit, the
> 1st change is adjusting the context due to lacking the free_irq()
> in the jammy, free_irq() is to fix another issue which is not
> relevant to this CVE case, the 2nd change is adding scmi_free_channel()
> calling if scmi_info is NULL. From the comment in the patch, different
> protocols might share the same chan info, and id might be specific to
> different protocols. If scmi_info is NULL, we don't need to set those
> pointers to NULL again, but we need to call scmi_free_channel()->idr_remove()
> to remove this protocol's id. And it is safe to call idr_remove() even
> if the id was already removed previously, this is the comment for
> idr_remove(): If the ID was not previously in the IDR, this function
> returns %NULL.
> 
> scmi_free_channel() is removed due to this commit: 05a2801d8b90
> ("firmware: arm_scmi: Use dedicated devices to initialize channels"),
> it is hard to backport this commit to jammy, hence we need to keep
> scmi_free_channel() in jammy and handle it in this CVE case.
> 
> 
> [Fix]
> 
> Noble:  Done
> Jammy:  Backported from mainline v6.3-rc1, see explanation in [Backport]
> Focal:  not affected
> Bionic: Not affected
> Xenial: Not affected
> Trusty: Not affected
> 
> [Test Case]
> 
> Compile and boot test.
> 
> And Tested the patched kernel on an ARM64 board, this board enables
> scmi based cpufreq, after running the patched kernel, the scmi-cpufreq
> could work as well as before.
> 
> bluebox at ubuntu-s32g399ardb3:/sys/devices/system/cpu/cpufreq/policy0$ cat *
> 0 1 2 3 4 5 6 7
> cat: cpuinfo_cur_freq: Permission denied
> 1300000
> 48148
> 4294967295
> 0 1 2 3 4 5 6 7
> 48148 54166 61904 72222 86666 104000 136842 200000 371428 1300000
> powersave performance schedutil
> 1300000
> scmi
> schedutil
> 1300000
> 48148
> <unsupported>
> cat: schedutil: Is a directory
> 
> 
> 
> [Where problems could occur]
> 
> The change is on scmi driver, if there is regression, it could impact
> scmi itself and some other drivers based on scmi like cpufreq. But the
> likely of regression is very low, the change is straightforward and
> simple, and I tested the patched kernel on an ARM64 platform with scmi
> cpufreq driver enabled,  everything worked well.
> 
> 
> Andre Przywara (1):
>    firmware: arm_scmi: Fix double free in SMC transport cleanup path
> 
>   drivers/firmware/arm_scmi/smc.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 


-- 
--
Thibault