linux-kernel - Re: [PATCH] ASoC: dapm: Check for NULL widget in dapm_update_dai

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] [day] [month] [year] [list]

Message-id: <b61f2293-6f48-45f3-ad0b-42bc2a214a76@samsung.com>
Date:   Wed, 06 Feb 2019 16:47:22 +0100
From:   Sylwester Nawrocki <s.nawrocki@...sung.com>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH] ASoC: dapm: Check for NULL widget in
 dapm_update_dai_unlocked

On 2/6/19 12:45, Charles Keepax wrote:
> Looking through I think this is an unrelated issue. Assuming the
> driver in question is (sound/soc/samsung/i2s.c). Inside
> i2s_trigger, there is a call to config_setup(i2s), which in turn
> will call clk_get_rate if i2s->quirks contains the flag
> QUIRK_NO_MUXPSR.
> 
> The trigger callback can be made from an atomic context and
> clk_get_rate will take the prepare lock of the clock. The clock
> prepare lock is always a mutex which shouldn't be locked from an
> atomic context. So it seems like this will fail whenever that
> QUIRK_NO_MUXPSR flag is set, no idea what causes that to be set.
> 
> It looks like this bug was introduced by this change:
> 
> 647d04f8e07a ("ASoC: samsung: i2s: Ensure the RCLK rate is properly determined")

Thanks or having a look at this. I somehow overlooked it before, there 
are multiple issues with that clk_get_rate() call.  Apart of the problem 
described above config_setup() is already called with the i2s->lock 
spinlock held, exactly same spinlock that is passed to the clock API 
when registering a clock of which we try to get rate. :/ Presumably 
it works due to clk rate caching.

Whether QUIRK_NO_MUXPSR flag is set or not depends on the HW type, 
it is not set for modern SoCs so most of the time we will hit the
problem in I2S master configurations.

As we are not using set_sysclk() of the CPU DAI I'm thinking about 
moving the clk_get_rate() call to the CPU DAI's hw_params() callback. 
The clock rate may be changed in hw_params() of an ASoC machine driver, 
and the CPU DAI needs to adjust to those changes.

It feels like locking in that driver might need revisiting, there is
quite a lot happening while holding a spinlock.

-- 
Thanks,
Sylwester