NACK: [SRU][J][PATCH 0/2] CVE-2024-26893

[Hui Wang]

Sorry, need more change.

On 9/10/24 10:37, Hui Wang wrote:
> [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]
>
> The 0001-xxx.patch is a prerequisite patch to avoid context conflict
> when applying the major fixing patch (0002-xxx.patch), and meanwhile
> the 0001-xxx.patch could fix a known issue although this issue is
> unrelevant to this CVE case. So it is not harmful to introduce this
> patch with this CVE case.
>
> For major fixing patch on this CVE case, I didn't just cherry-pick
> the original patch since the function in question is differnt from
> jammy and v6.3, we need to handle scmi_free_channel() in the jammy.
>
> 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.
>
> This backporting adds scmi_free_channel() calling if scmi_info
> is NULL. From the comment in this patch, different protocols might
> share the same chan info, and id is might 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.
>
>
> [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
>
> Cristian Marussi (1):
>    firmware: arm_scmi: Fix chan_free cleanup on SMC
>
>   drivers/firmware/arm_scmi/smc.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>