diff mbox

[1/1] Preventive fix in sound module

Message ID 1531908468-1352-1-git-send-email-srikanth.h@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Srikanth Korangala Hari July 18, 2018, 10:07 a.m. UTC 
Signed-off-by: Srikanth K H <srikanth.h@samsung.com>
---
 sound/core/timer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Srikanth Korangala Hari July 18, 2018, 10:58 a.m. UTC | #1 
>> 

>> 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
Takashi Iwai July 18, 2018, 12:14 p.m. UTC | #2 
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
diff mbox

Patch

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: