ALSA: pcm: Need to check whether runtime is valid or not

Message ID 20210512044323.106673-1-chanho61.park@samsung.com
State New
Headers show
Series
  • ALSA: pcm: Need to check whether runtime is valid or not
Related show

Commit Message

Chanho Park May 12, 2021, 4:43 a.m.
From: eunwoo kim <ew.kim@samsung.com>

Since snd_pcm_ioctl_sw_params_compat has no runtime variable check,
if application call the ioctl after close, it can make kernel crash.
So, snd_pcm_ioctl_sw_params_compat needs to check the runtime variable
at the beginning of the function.

Signed-off-by: eunwoo kim <ew.kim@samsung.com>
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 sound/core/pcm_compat.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Takashi Iwai May 12, 2021, 7:08 a.m. | #1
On Wed, 12 May 2021 06:43:23 +0200,
Chanho Park wrote:
> 
> From: eunwoo kim <ew.kim@samsung.com>
> 
> Since snd_pcm_ioctl_sw_params_compat has no runtime variable check,
> if application call the ioctl after close, it can make kernel crash.
> So, snd_pcm_ioctl_sw_params_compat needs to check the runtime variable
> at the beginning of the function.

In principle, you cannot call ioctl for an already closed file.
Or do you mean other code path?


Takashi
EUNWOO KIM May 19, 2021, 11:16 p.m. | #2
> > > Sender : Takashi Iwai <tiwai@suse.de>

   > > > Date : 2021-05-12 16:08 (GMT+9)

   > > > Title : Re: [PATCH] ALSA: pcm: Need to check whether runtime is v
   alid or not

   > > >

   > > > On Wed, 12 May 2021 06:43:23 +0200,

   > > > Chanho Park wrote:

   > > > >

   > > > > From: eunwoo kim <ew.kim@samsung.com>

   > > > >

   > > > > Since snd_pcm_ioctl_sw_params_compat has no runtime variable ch
   eck,

   > > > > if application call the ioctl after close, it can make kernel c
   rash.

   > > > > So, snd_pcm_ioctl_sw_params_compat needs to check the runtime v
   ariable

   > > > > at the beginning of the function.

   > > >

   > > > > In principle, you cannot call ioctl for an already closed file.

   > > > > Or do you mean other code path?

   > > >

   > > > Yes, other code path such as application layer or alsa framwork l
   ayer.

   > >

   > > But how can it go over the 32bit compat ioctl layer...?

   > > It's always tied with a file object, so the runtime object is

   > > assured.

   > >

   > > > > Yes, I think so too. but in fact, some case can call 32bit comp
   at ioctl.

   > >

   > > > In our case, suspend call is releasing all running pcm device. ot
   her side some application want to start music.

   > > > it make critical section between suspend and pcm_open from applic
   ation.

   > > > this is error and we have to solve this problem without this patc
   h.

   > > > but I think kernel don't make kernel crash even if application or
    system architecture have problem.

   > >

   > > I guess you're scratching a wrong surface.

   > >

   > > > could you tell me more detail? why do you think that.

   > > > In 64bit native case, it didn't make kernel crash on same test ca
   se. because native always check runtime in ioctl.

   >

   > > Try to put WARN_ON() there.  If you can catch the real case, it'd b
   e

   > > worth to merge.  Otherwise, it's just a wrong place to look at.

   >

   > I already found root cause in our issue case. And I am fixing this is
   sue via changing sequence.

   >

   > I think that you want to know issue case because why this patch need.

   > So I explain more about our case.

   > 1. application playback sound in android.

   > 2. call pcm_open in tinyalsa.

   > 3. call file open and hw param in pcm_open

   > 4. go to suspend

   > 5. freezing all task before calling sw param in pcm_open.

   > 6. kernel layer try to go suspend

   > 7. release all running substream, because all HW IPs must to go idle
   for power save mode.

   > -> detach runtime from substream.

   > 8. go to resume.

   > 9. call sw param in pcm_open.

   > 10-1. native case : has runtime check in ioctl -> return error -> aud
   io hal layer recovery hw connect.

   > 10-2. 32bit compat case : has not runtime check -> runtime is not val
   id -> make kernel crash.

   >

   > - if AAOS can support wakelock, we can postpone suspend until finishe
   d pcm_open. but AAOS didn't support wakelock.


   > I'm afraid that the approach is wrong.  It breaks the fundamental PCM

   > state change rule.


   Yes, that is wrong approach I know. So I am changing that.


   > I guess it's a downstream driver that does that?  If so, tweaking

   > something superfluous in the upstream code is unacceptable.

   > BTW, if you want further discussions, please don't drop Cc to

   > alsa-devel ML.


   Safety is very important in automotive. so it must not to make kernel
   crash even if someone make wrong code. because it can connect to
   accident.

   If 32bit compat don't need checking-runtime, why do pcm_native check
   runtime?


   thanks,


   Takashi


   [update?userid=ew.kim&do=bWFpbElEPTIwMjEwNTE5MjMxNjE1ZXBjbXMycDE4Y2NlMT
   Q1YzJhOWMwZDk5ZmViZjMxNDBjMTY3ZDIyNiZyZWNpcGllbnRBZGRyZXNzPWFsc2EtZGV2Z
   WxAYWxzYS1wcm9qZWN0Lm9yZw__]
Takashi Iwai May 20, 2021, 8:10 a.m. | #3
On Thu, 20 May 2021 01:16:15 +0200,
EUNWOO KIM wrote:
> 
> > > > Sender : Takashi Iwai <tiwai@suse.de>
> 
> > > > Date : 2021-05-12 16:08 (GMT+9)
> 
> > > > Title : Re: [PATCH]
>  ALSA: pcm: Need to check whether runtime is valid or not
> 
> > > >  
> 
> > > > On Wed, 12 May 2021 06:43:23 +0200,
> 
> > > > Chanho Park wrote:
> 
> > > > > 
> 
> > > > > From: eunwoo kim <ew.kim@samsung.com>
> 
> > > > > 
> 
> > > > > Since snd_pcm_ioctl_sw_params_compat has no runtime variable check,
> 
> > > > > if application call the ioctl after close, it can make kernel crash.
> 
> > > > > So, snd_pcm_ioctl_sw_params_compat needs to check the runtime variable
> 
> > > > > at the beginning of the function.
> 
> > > >  
> 
> > > > > In principle, you cannot call ioctl for an already closed file.
> 
> > > > > Or do you mean other code path?
> 
> > > > 
> 
> > > > Yes, other code path such as application layer or alsa framwork layer.
> 
> > >  
> 
> > > But how can it go over the 32bit compat ioctl layer...?
> 
> > > It's always tied with a file object, so the runtime object is
> 
> > > assured.
> 
> > > 
> 
> > > > >
>  Yes, I think so too. but in fact, some case can call 32bit compat ioctl.
> 
> > >  
> 
> > > >
>  In our case, suspend call is releasing all running pcm device. other side some application want to start music.
> 
> > > > it make critical section between suspend and pcm_open from application.
> 
> > > > this is error and we have to solve this problem without this patch.
> 
> > > >
>  but I think kernel don't make kernel crash even if application or system architecture have problem.
> 
> > >  
> 
> > > I guess you're scratching a wrong surface.
> 
> > > 
> 
> > > > could you tell me more detail? why do you think that.
> 
> > > >
>  In 64bit native case, it didn't make kernel crash on same test case. because native always check runtime in ioctl.
> 
> >  
> 
> > > Try to put WARN_ON() there.  If you can catch the real case, it'd be
> 
> > > worth to merge.  Otherwise, it's just a wrong place to look at.
> 
> > 
> 
> >
>  I already found root cause in our issue case. And I am fixing this issue via changing sequence.
> 
> > 
> 
> > I think that you want to know issue case because why this patch need.
> 
> > So I explain more about our case.
> 
> > 1. application playback sound in android.
> 
> > 2. call pcm_open in tinyalsa.
> 
> > 3. call file open and hw param in pcm_open
> 
> > 4. go to suspend 
> 
> > 5. freezing all task before calling sw param in pcm_open.
> 
> > 6. kernel layer try to go suspend
> 
> >
>  7. release all running substream, because all HW IPs must to go idle for power save mode.
> 
> > -> detach runtime from substream.
> 
> > 8. go to resume.
> 
> > 9. call sw param in pcm_open.
> 
> > 10-1. native case : has runtime check in ioctl -> return error ->
>  audio hal layer recovery hw connect.
> 
> > 10-2. 32bit compat case : has not runtime check -> runtime is not valid ->
>  make kernel crash.
> 
> > 
> 
> >
>  - if AAOS can support wakelock, we can postpone suspend until finished pcm_open. but AAOS didn't support wakelock.
> 
> > I'm afraid that the approach is wrong.  It breaks the fundamental PCM
> 
> > state change rule.
> 
> Yes, that is wrong approach I know. So I am changing that.
> 
> > I guess it's a downstream driver that does that?  If so, tweaking
> 
> > something superfluous in the upstream code is unacceptable.
> 
> > BTW, if you want further discussions, please don't drop Cc to
> 
> > alsa-devel ML.
> 
> Safety is very important in automotive. so it must not to make kernel crash
> even if someone make wrong code. because it can connect to accident.

Shipping such a broken stuff on the car would be a more serious
problem.  Really.  If this kind of change is required, it means that
the driver has a fundamental design problem.

A wrong driver code can always crash a kernel, no matter how the core
stuff is written.  So, it might make sense to put some debug code, but
it'd be not meant as a "fix" but only for debugging.

> If 32bit compat don't need checking-runtime, why do pcm_native check runtime?

That code path can be called from OSS layer, so it's more complex.
We may drop the check as well, of course.  It's no mandatory check at
all.


Takashi
EUNWOO KIM May 24, 2021, 10:41 p.m. | #4
--------- Original Message ---------

   Sender : Takashi Iwai <tiwai@suse.de>

   Date : 2021-05-20 17:10 (GMT+9)

   Title : Re: [PATCH] ALSA: pcm: Need to check whether runtime is valid
   or not

   To : 김은우<ew.kim@samsung.com>

   CC : alsa-devel@alsa-project.org


   On Thu, 20 May 2021 01:16:15 +0200,

   EUNWOO KIM wrote:

   >

   > > > > Sender : Takashi Iwai <tiwai@suse.de>

   >

   > > > > Date : 2021-05-12 16:08 (GMT+9)

   >

   > > > > Title : Re: [PATCH]

   >  ALSA: pcm: Need to check whether runtime is valid or not

   >

   > > > >

   >

   > > > > On Wed, 12 May 2021 06:43:23 +0200,

   >

   > > > > Chanho Park wrote:

   >

   > > > > >

   >

   > > > > > From: eunwoo kim <ew.kim@samsung.com>

   >

   > > > > >

   >

   > > > > > Since snd_pcm_ioctl_sw_params_compat has no runtime variable
   check,

   >

   > > > > > if application call the ioctl after close, it can make kernel
    crash.

   >

   > > > > > So, snd_pcm_ioctl_sw_params_compat needs to check the runtime
    variable

   >

   > > > > > at the beginning of the function.

   >

   > > > >

   >

   > > > > > In principle, you cannot call ioctl for an already closed fil
   e.

   >

   > > > > > Or do you mean other code path?

   >

   > > > >

   >

   > > > > Yes, other code path such as application layer or alsa framwork
    layer.

   >

   > > >

   >

   > > > But how can it go over the 32bit compat ioctl layer...?

   >

   > > > It's always tied with a file object, so the runtime object is

   >

   > > > assured.

   >

   > > >

   >

   > > > > >

   >  Yes, I think so too. but in fact, some case can call 32bit compat io
   ctl.

   >

   > > >

   >

   > > > >

   >  In our case, suspend call is releasing all running pcm device. other
    side some application want to start music.

   >

   > > > > it make critical section between suspend and pcm_open from appl
   ication.

   >

   > > > > this is error and we have to solve this problem without this pa
   tch.

   >

   > > > >

   >  but I think kernel don't make kernel crash even if application or sy
   stem architecture have problem.

   >

   > > >

   >

   > > > I guess you're scratching a wrong surface.

   >

   > > >

   >

   > > > > could you tell me more detail? why do you think that.

   >

   > > > >

   >  In 64bit native case, it didn't make kernel crash on same test case.
    because native always check runtime in ioctl.

   >

   > >

   >

   > > > Try to put WARN_ON() there.  If you can catch the real case, it'd
    be

   >

   > > > worth to merge.  Otherwise, it's just a wrong place to look at.

   >

   > >

   >

   > >

   >  I already found root cause in our issue case. And I am fixing this i
   ssue via changing sequence.

   >

   > >

   >

   > > I think that you want to know issue case because why this patch nee
   d.

   >

   > > So I explain more about our case.

   >

   > > 1. application playback sound in android.

   >

   > > 2. call pcm_open in tinyalsa.

   >

   > > 3. call file open and hw param in pcm_open

   >

   > > 4. go to suspend

   >

   > > 5. freezing all task before calling sw param in pcm_open.

   >

   > > 6. kernel layer try to go suspend

   >

   > >

   >  7. release all running substream, because all HW IPs must to go idle
    for power save mode.

   >

   > > -> detach runtime from substream.

   >

   > > 8. go to resume.

   >

   > > 9. call sw param in pcm_open.

   >

   > > 10-1. native case : has runtime check in ioctl -> return error ->

   >  audio hal layer recovery hw connect.

   >

   > > 10-2. 32bit compat case : has not runtime check -> runtime is not v
   alid ->

   >  make kernel crash.

   >

   > >

   >

   > >

   >  - if AAOS can support wakelock, we can postpone suspend until finish
   ed pcm_open. but AAOS didn't support wakelock.

   >

   > > I'm afraid that the approach is wrong.  It breaks the fundamental P
   CM

   >

   > > state change rule.

   >

   > Yes, that is wrong approach I know. So I am changing that.

   >

   > > I guess it's a downstream driver that does that?  If so, tweaking

   >

   > > something superfluous in the upstream code is unacceptable.

   >

   > > BTW, if you want further discussions, please don't drop Cc to

   >

   > > alsa-devel ML.

   >

   > Safety is very important in automotive. so it must not to make kernel
    crash

   > even if someone make wrong code. because it can connect to accident.


   >> Shipping such a broken stuff on the car would be a more serious

   >> problem.  Really.  If this kind of change is required, it means that

   >> the driver has a fundamental design problem.


   >> Wrong driver code can always crash a kernel, no matter how the core

   >>
   stuff is written.  So, it might make sense to put some debug code, but

   >> it'd be not meant as a "fix" but only for debugging.


   > If 32bit compat don't need checking-runtime, why do pcm_native check
   runtime?


   >> That code path can be called from OSS layer, so it's more complex.

   >>
   We may drop the check as well, of course.  It's no mandatory check at
   all.


   if OSS layer means EL0 layer, we shouldn't think only EL0 layer.
   Because virtualization system is more complex than before.
   In current google conference, we can see virtio system for automotive.

   It means that we should handle to transfer between virtio-snd and alsa
   core in EL1 layer.
   so we have to concern any approach from kernel layer.
   I think we should avoid potential problems that may arise at now.


   Takashi


   [update?userid=ew.kim&do=bWFpbElEPTIwMjEwNTI0MjI0MTM0ZXBjbXMycDI3N2I1Nj
   EwYWNlMDY5ZmIzODBiZjQwNzkwMTNjYjVmNiZyZWNpcGllbnRBZGRyZXNzPWFsc2EtZGV2Z
   WxAYWxzYS1wcm9qZWN0Lm9yZw__]

Patch

diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 590a46a9e78d..ff0de4252ff4 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -90,6 +90,9 @@  static int snd_pcm_ioctl_sw_params_compat(struct snd_pcm_substream *substream,
 	snd_pcm_uframes_t boundary;
 	int err;
 
+	if (!substream->runtime)
+		return -ENOTTY;
+
 	memset(&params, 0, sizeof(params));
 	if (get_user(params.tstamp_mode, &src->tstamp_mode) ||
 	    get_user(params.period_step, &src->period_step) ||