| Message ID | 20240910084856.2355123-1-hui.wang@canonical.com |
|---|---|
| Headers | show |
| Series | CVE-2024-26893 | expand |
On 10-09-24 16:48:55, 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. Thank you for testing on actual hardware. Much appreciated. Acked-by: Cengiz Can <cengiz.can@canonical.com> > > 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@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(+) > > -- > 2.34.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Acked-by: Thibault Ferrante <thibault.ferrante@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@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(+) >
On 10.09.24 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@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(+) > Applied to jammy:linux/master-next. Thanks. -Stefan
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@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(+)