linux-kernel - Re: [PATCH 1/3] firmware: arm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0f122a88-e2e1-2139-1c2d-095f684a5701@arm.com>
Date:   Mon, 6 Jan 2020 15:56:51 +0000
From:   James Morse <james.morse@....com>
To:     luanshi <zhangliguang@...ux.alibaba.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] firmware: arm_sdei: fix possible deadlock

Hi Luanshi!

On 23/12/2019 14:22, luanshi wrote:
> From: Liguang Zhang <zhangliguang@...ux.alibaba.com>
> 
> We call sdei_reregister_event() with sdei_list_lock held but
> _sdei_event_register() and sdei_event_destroy() also acquires
> sdei_list_lock thus creating A-A deadlock.

Ooer. This was clearly never tested properly!
The hibernate support got plenty of testing, but it must have been with only private
events. Hibernate+SDEI with a side-order of cpuhp is a niche sport.

> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a479023..b122927 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
>  
>  	lockdep_assert_held(&sdei_events_lock);
>  
> -	err = _sdei_event_register(event);
> +	err = sdei_api_event_register(event->event_num,
> +				       sdei_entry_point,
> +				       event->registered,
> +				       SDEI_EVENT_REGISTER_RM_ANY, 0);

I don't like pushing these 'api' calls further out creating more of them...

The root of the problem is the reregister/reenable values are protected by the same lock
as the list, _sdei_event_register() needs to manipulate these, which it can't do from
something that is walking the list.

The list lock is a spin_lock() because the cpuhp callbacks happen too early for taking
mutexes, (fairly sure). Those callbacks don't hit this because they skip shared events.

As the simplest fix for stable, could we add another spin_lock inside struct sdei_event to
independently protect the reregister/renable values? This would always be taken last, and
removes the double-lock.

Was this from inspection, or is there some tool I should be running?!
(my testing obviously missed it)

Thanks,

James