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

[Bethany Jamison]

On 5/10/24 8:07 AM, Stefan Bader wrote:
> On 07.05.24 21:31, Bethany Jamison wrote:
>> [Impact]
>>
>>   In the Linux kernel, the following vulnerability has been resolved:
>>
>>   firmware: arm_scmi: Fix double free in SMC transport cleanup path
>>
>>   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:
>>
>>       | scmi_protocol scmi_dev.1: Enabled polling mode TX channel -
>>   prot_id:16
>>       | arm-scmi firmware:scmi: SCMI Notifications - Core Enabled.
>>       | arm-scmi firmware:scmi: unable to communicate with SCMI
>>       | Unable to handle kernel NULL pointer dereference at virtual 
>> address
>>   0000000000000000
>>       | Mem abort info:
>>       |   ESR = 0x0000000096000004
>>       |   EC = 0x25: DABT (current EL), IL = 32 bits
>>       |   SET = 0, FnV = 0
>>       |   EA = 0, S1PTW = 0
>>       |   FSC = 0x04: level 0 translation fault
>>       | Data abort info:
>>       |   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
>>       |   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>>       |   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>>       | user pgtable: 4k pages, 48-bit VAs, pgdp=0000000881ef8000
>>       | [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
>>       | Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>>       | Modules linked in:
>>       | CPU: 4 PID: 1 Comm: swapper/0 Not tainted
>> 6.7.0-rc2-00124-g455ef3d016c9-dirty #793
>>       | Hardware name: FVP Base RevC (DT)
>>       | pstate: 61400009 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
>>       | pc : smc_chan_free+0x3c/0x6c
>>       | lr : smc_chan_free+0x3c/0x6c
>>       | Call trace:
>>       |  smc_chan_free+0x3c/0x6c
>>       |  idr_for_each+0x68/0xf8
>>       |  scmi_cleanup_channels.isra.0+0x2c/0x58
>>       |  scmi_probe+0x434/0x734
>>       |  platform_probe+0x68/0xd8
>>       |  really_probe+0x110/0x27c
>>       |  __driver_probe_device+0x78/0x12c
>>       |  driver_probe_device+0x3c/0x118
>>       |  __driver_attach+0x74/0x128
>>       |  bus_for_each_dev+0x78/0xe0
>>       |  driver_attach+0x24/0x30
>>       |  bus_add_driver+0xe4/0x1e8
>>       |  driver_register+0x60/0x128
>>       |  __platform_driver_register+0x28/0x34
>>       |  scmi_driver_init+0x84/0xc0
>>       |  do_one_initcall+0x78/0x33c
>>       |  kernel_init_freeable+0x2b8/0x51c
>>       |  kernel_init+0x24/0x130
>>       |  ret_from_fork+0x10/0x20
>>       | Code: f0004701 910a0021 aa1403e5 97b91c70 (b9400280)
>>       | ---[ end trace 0000000000000000 ]---
>>
>>   Simply check for the struct pointer being NULL before trying to access
>>   its members, to avoid this situation.
>>
>>   This was found when a transport doesn't really work (for instance 
>> no SMC
>>   service), the probe routines then tries to clean up, and triggers a 
>> crash.
>>
>> [Fix]
>>
>> Mantic:    Clean cherry-pick from linux-6.6.y
>> Jammy:    Backported - I added the if-statement from the fix commit 
>> as is
>>     ignoring the context conflict (missing free_irq if-statement).
>> Focal:    not-affected
>> Bionic:    not-affected
>> Xenial:    not-affected
>> Trusty:    not-affected
>>
>> [Test Case]
>>
>> Compile and boot tested.
>>
>> [Where problems could occur]
>>
>> This fix affects those who use the ARM SCMI (System Control and
>> Management Interface) driver, an issue with this fix would be visable to
>> the user via a system crash.
>>
>> Andre Przywara (1):
>>    firmware: arm_scmi: Fix double free in SMC transport cleanup path
>>
>>   drivers/firmware/arm_scmi/smc.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
> For Jammy the context of the patch is not really conclusive and 
> looking at the code I am not sure whether this applies or might even 
> be harmful. In Mantic some irq gets released and apart from that only 
> two pointers get set to NULL. Then the function returns.
> In Jammy however there is no irq release and the function does not 
> immediately return after having set the pointers. Instead there is a 
> call to scmi_free_channel() which in turn calls idr_remove(). It is 
> rather hard to say whether skipping that is equal to not calling 
> free_irq().
>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>

For Jammy - I agree I also had some questions about the right way to 
apply this - I tracked down a commit 
(d1ff11d7ad8704f8d615f6446041c221b2d2ec4d) that adds the free_irq 
section but it doesn't build with cbd - I'm not sure the best way to go 
about fixing this for Jammy.