* [PATCH 1/1] Preventive fix in sound module
[not found] <CGME20180718100741epcas1p393bea852d102e903ab6a48ff952761db@epcas1p3.samsung.com>
@ 2018-07-18 10:07 ` Srikanth K H
2018-07-18 10:16 ` Takashi Iwai
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Srikanth K H @ 2018-07-18 10:07 UTC (permalink / raw)
To: perex, tiwai, elfring, ben.hutchings, viro, keescook, alsa-devel,
linux-kernel
Cc: cpgs, srikanth.h
Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
---
sound/core/timer.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c
index b6f076bb..c7be4f1 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -1192,10 +1192,12 @@ static void snd_timer_proc_read(struct snd_info_entry *entry,
break;
case SNDRV_TIMER_CLASS_CARD:
snd_iprintf(buffer, "C%i-%i: ",
- timer->card->number, timer->tmr_device);
+ timer->card ? timer->card->number : -1,
+ timer->tmr_device);
break;
case SNDRV_TIMER_CLASS_PCM:
- snd_iprintf(buffer, "P%i-%i-%i: ", timer->card->number,
+ snd_iprintf(buffer, "P%i-%i-%i: ",
+ timer->card ? timer->card->number : -1,
timer->tmr_device, timer->tmr_subdevice);
break;
default:
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] Preventive fix in sound module
2018-07-18 10:07 ` [PATCH 1/1] Preventive fix in sound module Srikanth K H
@ 2018-07-18 10:16 ` Takashi Iwai
[not found] ` <CGME20180718100741epcas1p393bea852d102e903ab6a48ff952761db@epcms5p1>
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcas2p3.samsung.com>
2 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-07-18 10:16 UTC (permalink / raw)
To: Srikanth K H
Cc: alsa-devel, keescook, ben.hutchings, perex, elfring,
linux-kernel, viro, cpgs
On Wed, 18 Jul 2018 12:07:48 +0200,
Srikanth K H wrote:
>
> Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
What does this fix, and above all, why is this needed?
thanks,
Takashi
> ---
> sound/core/timer.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index b6f076bb..c7be4f1 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -1192,10 +1192,12 @@ static void snd_timer_proc_read(struct snd_info_entry *entry,
> break;
> case SNDRV_TIMER_CLASS_CARD:
> snd_iprintf(buffer, "C%i-%i: ",
> - timer->card->number, timer->tmr_device);
> + timer->card ? timer->card->number : -1,
> + timer->tmr_device);
> break;
> case SNDRV_TIMER_CLASS_PCM:
> - snd_iprintf(buffer, "P%i-%i-%i: ", timer->card->number,
> + snd_iprintf(buffer, "P%i-%i-%i: ",
> + timer->card ? timer->card->number : -1,
> timer->tmr_device, timer->tmr_subdevice);
> break;
> default:
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: Re: [PATCH 1/1] Preventive fix in sound module
[not found] ` <CGME20180718100741epcas1p393bea852d102e903ab6a48ff952761db@epcms5p1>
@ 2018-07-18 10:58 ` Srikanth Korangala Hari
2018-07-18 12:14 ` Takashi Iwai
0 siblings, 1 reply; 7+ messages in thread
From: Srikanth Korangala Hari @ 2018-07-18 10:58 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, keescook, ben.hutchings, perex, elfring,
linux-kernel, viro, CPGS
>>
>> Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
>What does this fix, and above all, why is this needed?
Hi,
When the sound driver creates the timer without sound card object, then while reading the sound info entry the timer object’s card information is dereferenced without checking for NULL pointer which will result for kernel panic. I tried to simulate this scenario and got below call stack,
[ 36.668] E/DEVKMSG (P 0, T 0): Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 36.668] E/DEVKMSG (P 0, T 0): pgd = e52f0000
[ 36.668] E/DEVKMSG (P 0, T 0): [00000000] *pgd=00000000
[ 36.668] E/DEVKMSG (P 0, T 0): Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[ 36.668] E/DEVKMSG (P 0, T 0): Modules linked in:
[ 36.668] E/DEVKMSG (P 0, T 0): CPU: 1 PID: 1258 Comm: cat Tainted: G W 3.10.65-00121-g83e9b9b-dirty #54-Tizen
[ 36.668] E/DEVKMSG (P 0, T 0): task: e653aec0 ti: e52ec000 task.ti: e52ec000
[ 36.668] E/DEVKMSG (P 0, T 0): PC is at snd_timer_proc_read+0x104/0x278
[ 36.668] E/DEVKMSG (P 0, T 0): LR is at snd_timer_proc_read+0xec/0x278
[ 36.668] E/DEVKMSG (P 0, T 0): pc : [<c0527cfc>] lr : [<c0527ce4>] psr: 60040013\x0asp : e52eded0 ip : 00000000 fp : 10624dd3
[ 36.668] E/DEVKMSG (P 0, T 0): r10: c08ded6c r9 : e49e3bd8 r8 : c074f518
[ 36.668] E/DEVKMSG (P 0, T 0): r7 : c0afbae4 r6 : eb95a000 r5 : e49e3240 r4 : eb257e00
[ 36.668] E/DEVKMSG (P 0, T 0): r3 : 00000000 r2 : 00000000 r1 : c0987cd7 r0 : e49e3240
[ 36.668] E/DEVKMSG (P 0, T 0): Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 36.668] E/DEVKMSG (P 0, T 0): Control: 10c53c7d Table: a52f006a DAC: 00000015
Hence this is a preventive patch to avoid kernel panic in case if the card object passed to timer function is NULL. This would not happen in normal case, but in case of buggy scenario this would results in kernel panic rather than graceful exit.
thanks,
srikanth
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] Preventive fix in sound module
2018-07-18 10:58 ` Srikanth Korangala Hari
@ 2018-07-18 12:14 ` Takashi Iwai
0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-07-18 12:14 UTC (permalink / raw)
To: srikanth.h
Cc: alsa-devel, keescook, ben.hutchings, perex, elfring,
linux-kernel, viro, CPGS
On Wed, 18 Jul 2018 12:58:20 +0200,
Srikanth Korangala Hari wrote:
>
>
> >>
> >> Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
>
> >What does this fix, and above all, why is this needed?
>
> Hi,
>
> When the sound driver creates the timer without sound card object, then while reading the sound info entry the timer object’s card information is dereferenced without checking for NULL pointer which will result for kernel panic. I tried to simulate this scenario and got below call stack,
> [ 36.668] E/DEVKMSG (P 0, T 0): Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 36.668] E/DEVKMSG (P 0, T 0): pgd = e52f0000
> [ 36.668] E/DEVKMSG (P 0, T 0): [00000000] *pgd=00000000
> [ 36.668] E/DEVKMSG (P 0, T 0): Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> [ 36.668] E/DEVKMSG (P 0, T 0): Modules linked in:
> [ 36.668] E/DEVKMSG (P 0, T 0): CPU: 1 PID: 1258 Comm: cat Tainted: G W 3.10.65-00121-g83e9b9b-dirty #54-Tizen
> [ 36.668] E/DEVKMSG (P 0, T 0): task: e653aec0 ti: e52ec000 task.ti: e52ec000
> [ 36.668] E/DEVKMSG (P 0, T 0): PC is at snd_timer_proc_read+0x104/0x278
> [ 36.668] E/DEVKMSG (P 0, T 0): LR is at snd_timer_proc_read+0xec/0x278
> [ 36.668] E/DEVKMSG (P 0, T 0): pc : [<c0527cfc>] lr : [<c0527ce4>] psr: 60040013\x0asp : e52eded0 ip : 00000000 fp : 10624dd3
> [ 36.668] E/DEVKMSG (P 0, T 0): r10: c08ded6c r9 : e49e3bd8 r8 : c074f518
> [ 36.668] E/DEVKMSG (P 0, T 0): r7 : c0afbae4 r6 : eb95a000 r5 : e49e3240 r4 : eb257e00
> [ 36.668] E/DEVKMSG (P 0, T 0): r3 : 00000000 r2 : 00000000 r1 : c0987cd7 r0 : e49e3240
> [ 36.668] E/DEVKMSG (P 0, T 0): Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 36.668] E/DEVKMSG (P 0, T 0): Control: 10c53c7d Table: a52f006a DAC: 00000015
>
> Hence this is a preventive patch to avoid kernel panic in case if the card object passed to timer function is NULL. This would not happen in normal case, but in case of buggy scenario this would results in kernel panic rather than graceful exit.
The timer->card must be associated with both entries of
SNDRV_TIMER_CLASS_CARD and SNDRV_TIMER_CLASS_PCM.
IOW, if a timer object is created without the card but for these
classes, it's already a bug. Papering over it doesn't give any
benefits. At most it should be with WARN_ON(), but I guess here is a
wrong place to add the check. The check should be done at the object
creation time instead.
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCHv2 1/1] Preventive fix in sound module
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcas2p3.samsung.com>
@ 2018-07-18 15:07 ` Srikanth K H
2018-07-18 15:24 ` Takashi Iwai
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcms5p8>
0 siblings, 2 replies; 7+ messages in thread
From: Srikanth K H @ 2018-07-18 15:07 UTC (permalink / raw)
To: perex, tiwai, elfring, ben.hutchings, viro, keescook, alsa-devel,
linux-kernel
Cc: cpgs, srikanth.h
If the timer object is created without the card for entries
"SNDRV_TIMER_CLASS_CARD" and "SNDRV_TIMER_CLASS_PCM", then
while reading the sound info entry in function
"snd_timer_proc_read" the card information is directly
dereferenced without checking for NULL and hence kernel
panic occur. So as preventive measure while the creating
the sound timer object is created the card information
availability is checked for the mentioned entries and
returned error if its NULL.
Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
---
sound/core/timer.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c
index c7be4f1..06f734f 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -883,6 +883,11 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
if (snd_BUG_ON(!tid))
return -EINVAL;
+ if (tid->dev_class == SNDRV_TIMER_CLASS_CARD ||
+ tid->dev_class == SNDRV_TIMER_CLASS_PCM) {
+ if (WARN_ON(!card))
+ return -EINVAL;
+ }
if (rtimer)
*rtimer = NULL;
timer = kzalloc(sizeof(*timer), GFP_KERNEL);
@@ -1192,12 +1197,10 @@ static void snd_timer_proc_read(struct snd_info_entry *entry,
break;
case SNDRV_TIMER_CLASS_CARD:
snd_iprintf(buffer, "C%i-%i: ",
- timer->card ? timer->card->number : -1,
- timer->tmr_device);
+ timer->card->number, timer->tmr_device);
break;
case SNDRV_TIMER_CLASS_PCM:
- snd_iprintf(buffer, "P%i-%i-%i: ",
- timer->card ? timer->card->number : -1,
+ snd_iprintf(buffer, "P%i-%i-%i: ", timer->card->number,
timer->tmr_device, timer->tmr_subdevice);
break;
default:
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCHv2 1/1] Preventive fix in sound module
2018-07-18 15:07 ` [PATCHv2 " Srikanth K H
@ 2018-07-18 15:24 ` Takashi Iwai
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcms5p8>
1 sibling, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-07-18 15:24 UTC (permalink / raw)
To: Srikanth K H
Cc: alsa-devel, keescook, ben.hutchings, perex, elfring,
linux-kernel, viro, cpgs
On Wed, 18 Jul 2018 17:07:00 +0200,
Srikanth K H wrote:
>
> If the timer object is created without the card for entries
> "SNDRV_TIMER_CLASS_CARD" and "SNDRV_TIMER_CLASS_PCM", then
> while reading the sound info entry in function
> "snd_timer_proc_read" the card information is directly
> dereferenced without checking for NULL and hence kernel
> panic occur. So as preventive measure while the creating
> the sound timer object is created the card information
> availability is checked for the mentioned entries and
> returned error if its NULL.
>
> Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
> ---
> sound/core/timer.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index c7be4f1..06f734f 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -883,6 +883,11 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
>
> if (snd_BUG_ON(!tid))
> return -EINVAL;
> + if (tid->dev_class == SNDRV_TIMER_CLASS_CARD ||
> + tid->dev_class == SNDRV_TIMER_CLASS_PCM) {
> + if (WARN_ON(!card))
> + return -EINVAL;
> + }
> if (rtimer)
> *rtimer = NULL;
> timer = kzalloc(sizeof(*timer), GFP_KERNEL);
> @@ -1192,12 +1197,10 @@ static void snd_timer_proc_read(struct snd_info_entry *entry,
> break;
> case SNDRV_TIMER_CLASS_CARD:
> snd_iprintf(buffer, "C%i-%i: ",
> - timer->card ? timer->card->number : -1,
> - timer->tmr_device);
> + timer->card->number, timer->tmr_device);
> break;
> case SNDRV_TIMER_CLASS_PCM:
> - snd_iprintf(buffer, "P%i-%i-%i: ",
> - timer->card ? timer->card->number : -1,
> + snd_iprintf(buffer, "P%i-%i-%i: ", timer->card->number,
> timer->tmr_device, timer->tmr_subdevice);
> break;
The checks in proc are moot if we guarantee the non-NULL card at
snd_timer_new() in the above.
So it's not about fixing in sound module. It serves right. Your
patch would add a sanity check to catch a buggy code in the caller
side.
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: Re: [PATCHv2 1/1] Preventive fix in sound module
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcms5p8>
@ 2018-07-19 5:16 ` Srikanth Korangala Hari
0 siblings, 0 replies; 7+ messages in thread
From: Srikanth Korangala Hari @ 2018-07-19 5:16 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, keescook, ben.hutchings, perex, elfring,
linux-kernel, viro, CPGS
> The checks in proc are moot if we guarantee the non-NULL card at
> snd_timer_new() in the above.
> So it's not about fixing in sound module. It serves right. Your
> patch would add a sanity check to catch a buggy code in the caller
> side.
You are right Takashi, as you said this changes will catch buggy code in caller side.
Thank you for your respose.
Thanks,
Srikanth
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-19 5:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180718100741epcas1p393bea852d102e903ab6a48ff952761db@epcas1p3.samsung.com>
2018-07-18 10:07 ` [PATCH 1/1] Preventive fix in sound module Srikanth K H
2018-07-18 10:16 ` Takashi Iwai
[not found] ` <CGME20180718100741epcas1p393bea852d102e903ab6a48ff952761db@epcms5p1>
2018-07-18 10:58 ` Srikanth Korangala Hari
2018-07-18 12:14 ` Takashi Iwai
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcas2p3.samsung.com>
2018-07-18 15:07 ` [PATCHv2 " Srikanth K H
2018-07-18 15:24 ` Takashi Iwai
[not found] ` <CGME20180718150653epcas2p3c2f0e36569529df72ce0b79a22867eac@epcms5p8>
2018-07-19 5:16 ` Srikanth Korangala Hari
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).