The commit:
ASoC: pcm: free path list before exiting from error conditions
is broken and must not be applied to stable kernels, and needs reverting
for 3.14.
Let's look at the three places this has been added:
paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_PLAYBACK, &list);
if (paths < 0) {
+ dpcm_path_put(&list);
paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_CAPTURE, &list);
if (paths < 0) {
+ dpcm_path_put(&list);
if (dpcm_path_get(fe, stream, &list) <= 0) {
+ dpcm_path_put(&list);
and ask the basic question - under what circumstances does dpcm_path_get()
return a value less than zero?
Well, one case is this path:
int dpcm_path_get(struct snd_soc_pcm_runtime *fe,
int stream, struct snd_soc_dapm_widget_list **list_)
{
list = kzalloc(sizeof(struct snd_soc_dapm_widget_list) +
sizeof(struct snd_soc_dapm_widget *), GFP_KERNEL);
if (list == NULL)
return -ENOMEM;
and the result is that `list' in each of the three cases above is
uninitialised.
Another case is:
/* get number of valid DAI paths and their widgets */
paths = snd_soc_dapm_dai_get_connected_widgets(cpu_dai, stream, &list);
dev_dbg(fe->dev, "ASoC: found %d audio %s paths\n", paths,
stream ? "capture" : "playback");
*list_ = list;
return paths;
}
if snd_soc_dapm_dai_get_connected_widgets() returns negative, or in
the case of dpcm_fe_dai_open(), it returns zero. Can this return
negative? Well, snd_soc_dapm_dai_get_connected_widgets() selects
between calling is_connected_output_ep() and is_connected_input_ep(),
returning its value.
>From what I can see, these two functions do not return negative numbers
ever. They may return zero.
So, in the case of the first two additions, paths will only be negative
_if_ we failed to allocate the memory for `list' which means `list' does
*not* need freeing.
In the third case, let's look closer at the function:
struct snd_soc_dapm_widget_list *list;
if (dpcm_path_get(fe, stream, &list) <= 0) {
dpcm_path_put(&list);
dev_dbg(fe->dev, "ASoC: %s no valid %s route\n",
fe->dai_link->name, stream ? "capture" : "playback");
}
Note that this block does not exit, it continues on:
/* calculate valid and active FE <-> BE dpcms */
dpcm_process_paths(fe, stream, &list, 1);
and dpcm_process_paths dereferences &list, and ultimately dereferences
`list' itself. There's two bugs here - first, in the case of
dpcm_path_get() returning negative, if we survive dpcm_path_put() then
we're dereferencing an uninitialised pointer in this function. Secondly,
if dpcm_path_get() returned zero, we end up dereferencing memory which
has been freed.
The above function continues, and eventually we get here:
dpcm_clear_pending_state(fe, stream);
dpcm_path_put(&list);
which ends up trying a second time to put this list.
Therefore, this commit is 100% wrong in every regard, and it needs to be
reverted. However, in analysing this, there is the matter of the
unchecked failure of dpcm_path_get() when it returns negative, leaving
`list' completely uninitialised.
I noticed this because this commit leads to an oops in dpcm_add_paths()
as the free'd memory changes state beneath this function resulting in
a hard to debug oops.
Unable to handle kernel NULL pointer dereference at virtual address 00000004
pgd = d062c000
[00000004] *pgd=10609831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT ARM
Modules linked in: fuse bnep rfcomm bluetooth 6lowpan_iphc ext3 jbd ext2 snd_soc_spdif_tx m25p80 mtd orion_wdt snd_soc_kirkwood snd_soc_kirkwood_spdif
CPU: 0 PID: 1564 Comm: pulseaudio Not tainted 3.14.0+ #872
task: d05c2400 ti: d0628000 task.ti: d0628000
PC is at dpcm_add_paths.isra.7+0x148/0x164
LR is at dpcm_get_be+0x88/0x108
pc : [<c032c240>] lr : [<c032abe8>] psr: 600f0013
sp : d0629d18 ip : 00000000 fp : d5b1a000
r10: 00000000 r9 : d588c010 r8 : d5a99148
r7 : 00000001 r6 : d588c010 r5 : 00000000 r4 : d5a99140
r3 : 00000000 r2 : 00000000 r1 : c05485a0 r0 : d5bad800
Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
Control: 10c5387d Table: 1062c019 DAC: 00000015
Process pulseaudio (pid: 1564, stack limit = 0xd0628250)
...
Code: eaffffec e5983000 e5960000 e59f1014 (e5932004)
The oopsing instruction is from:
dev_err(fe->dev, "ASoC: no BE found for %s\n",
list->widgets[i]->name);
trying to load ->name, but list->widgets[1] has magically turned into
a NULL pointer mid-function.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html