[alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

Arnd Bergmann

16 Feb 2016 16 Feb '16

3:47 p.m.

When CONFIG_SND_JACK is disabled, the hdmi driver tries to create a new output jack structure and dereferences it even though the pointer is never assigned:

sound/pci/hda/patch_hdmi.c: In function 'generic_hdmi_build_jack': sound/pci/hda/patch_hdmi.c:2534:30: error: 'jack' may be used uninitialized in this function [-Werror=maybe-uninitialized] sound/pci/hda/patch_hdmi.c:2526:19: note: 'jack' was declared here

This uses a compile-time check to avoid calling a nonworking snd_jack_new() function, and sets an explicit NULL pointer instead.

Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: 788d441a164c ("ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling") --- sound/pci/hda/patch_hdmi.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index f4443b5fbf6e..c0872ad80aa0 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2526,6 +2526,11 @@ static int add_hdmi_jack_kctl(struct hda_codec *codec, struct snd_jack *jack; int err;

+ if (!IS_ENABLED(CONFIG_SND_JACK)) { + spec->pcm_rec[pcm_idx].jack = NULL; + return 0; + } + err = snd_jack_new(codec->card, name, SND_JACK_AVOUT, &jack, true, false); if (err < 0)

-- 2.7.0

Show replies by date

Takashi Iwai

16 Feb 16 Feb

4:49 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, 16 Feb 2016 15:47:28 +0100, Arnd Bergmann wrote:

...

When CONFIG_SND_JACK is disabled, the hdmi driver tries to create a new output jack structure and dereferences it even though the pointer is never assigned:

sound/pci/hda/patch_hdmi.c: In function 'generic_hdmi_build_jack': sound/pci/hda/patch_hdmi.c:2534:30: error: 'jack' may be used uninitialized in this function [-Werror=maybe-uninitialized] sound/pci/hda/patch_hdmi.c:2526:19: note: 'jack' was declared here

This uses a compile-time check to avoid calling a nonworking snd_jack_new() function, and sets an explicit NULL pointer instead.

Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: 788d441a164c ("ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling")

Thanks for the patch. But I think it's cleaner to fix Kconfig.

Thinking more of it, maybe splitting jack stuff as a separate module and does reverse-select to CONFIG_INPUT would be better. Then its users can select simply SND_JACK, and everything would fit.

An untested patch is below. Mark, what do you think?

Takashi

--- diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a2a1e24becc6..cf4058dde959 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -24,11 +24,9 @@ config SND_RAWMIDI config SND_COMPRESS_OFFLOAD tristate

-# To be effective this also requires INPUT - users should say: -# select SND_JACK if INPUT=y || INPUT=SND -# to avoid having to force INPUT on. config SND_JACK - bool + tristate + select INPUT

config SND_SEQUENCER tristate "Sequencer support" diff --git a/sound/core/Makefile b/sound/core/Makefile index 48ab4b8f8279..d16e2b0ba4fb 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -11,7 +11,8 @@ endif snd-$(CONFIG_ISA_DMA_API) += isadma.o snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o snd-$(CONFIG_SND_VMASTER) += vmaster.o -snd-$(CONFIG_SND_JACK) += ctljack.o jack.o + +snd-jack-objs := ctljack.o jack.o

snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_misc.o \ pcm_memory.o memalloc.o @@ -41,6 +42,7 @@ obj-$(CONFIG_SND_RTCTIMER) += snd-rtctimer.o obj-$(CONFIG_SND_PCM) += snd-pcm.o obj-$(CONFIG_SND_DMAENGINE_PCM) += snd-pcm-dmaengine.o obj-$(CONFIG_SND_RAWMIDI) += snd-rawmidi.o +obj-$(CONFIG_SND_JACK) += snd-jack.o

obj-$(CONFIG_SND_OSSEMUL) += oss/ obj-$(CONFIG_SND_SEQUENCER) += seq/ diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig index 8f6594a7d37f..32151d8c6bb8 100644 --- a/sound/pci/Kconfig +++ b/sound/pci/Kconfig @@ -866,7 +866,7 @@ config SND_VIRTUOSO select SND_OXYGEN_LIB select SND_PCM select SND_MPU401_UART - select SND_JACK if INPUT=y || INPUT=SND + select SND_JACK help Say Y here to include support for sound cards based on the Asus AV66/AV100/AV200 chips, i.e., Xonar D1, DX, D2, D2X, DS, DSX, diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index e94cfd5c69f7..bb02c2d48fd5 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -4,7 +4,7 @@ config SND_HDA tristate select SND_PCM select SND_VMASTER - select SND_JACK if INPUT=y || INPUT=SND + select SND_JACK select SND_HDA_CORE

config SND_HDA_INTEL diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 7ea66ee3653f..182d92efc7c8 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -6,7 +6,7 @@ menuconfig SND_SOC tristate "ALSA for SoC audio support" select SND_PCM select AC97_BUS if SND_SOC_AC97_BUS - select SND_JACK if INPUT=y || INPUT=SND + select SND_JACK select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER ---help---

Arnd Bergmann

5:09 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tuesday 16 February 2016 16:49:42 Takashi Iwai wrote:

...

Thanks for the patch. But I think it's cleaner to fix Kconfig.

Thinking more of it, maybe splitting jack stuff as a separate module and does reverse-select to CONFIG_INPUT would be better. Then its users can select simply SND_JACK, and everything would fit.

Adding 'select INPUT' is rather nasty, I think that can lead to circular dependencies, and would likely upset users of small embedded systems that want to use audio but don't want to use input.

Generally speaking, I would recommend never using 'select' on a user visible Kconfig symbol.

Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

Arnd

Takashi Iwai

5:18 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, 16 Feb 2016 17:09:43 +0100, Arnd Bergmann wrote:

...

On Tuesday 16 February 2016 16:49:42 Takashi Iwai wrote:

...
Thanks for the patch. But I think it's cleaner to fix Kconfig.

Thinking more of it, maybe splitting jack stuff as a separate module and does reverse-select to CONFIG_INPUT would be better. Then its users can select simply SND_JACK, and everything would fit.

Adding 'select INPUT' is rather nasty, I think that can lead to circular dependencies, and would likely upset users of small embedded systems that want to use audio but don't want to use input.

Fair enough.

...

Generally speaking, I would recommend never using 'select' on a user visible Kconfig symbol.

Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

Yes, I thought of that, too. If select is no good option, it's a good alternative, indeed.

Takashi

Mark Brown

5:38 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, Feb 16, 2016 at 05:18:29PM +0100, Takashi Iwai wrote:

...

Arnd Bergmann wrote:

...

...
Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

...

Yes, I thought of that, too. If select is no good option, it's a good alternative, indeed.

It's going to be a bunch of work to implement though.

Takashi Iwai

5:43 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, 16 Feb 2016 17:38:40 +0100, Mark Brown wrote:

...

On Tue, Feb 16, 2016 at 05:18:29PM +0100, Takashi Iwai wrote:

...
Arnd Bergmann wrote:

...
...
Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

...
Yes, I thought of that, too. If select is no good option, it's a good alternative, indeed.

It's going to be a bunch of work to implement though.

Is it? Which driver would be broken? Many ASoC drivers just ignore the return error completely. Some treats as a fatal error, and the behavior would change, yes. But I don't think that such a driver would work without CONFIG_SND_JACK properly in anyway.

Takashi

Arnd Bergmann

5:59 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tuesday 16 February 2016 16:38:40 Mark Brown wrote:

...

On Tue, Feb 16, 2016 at 05:18:29PM +0100, Takashi Iwai wrote:

...
Arnd Bergmann wrote:

...
...
Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

...
Yes, I thought of that, too. If select is no good option, it's a good alternative, indeed.

It's going to be a bunch of work to implement though.

I've already sent a v2 to change the snd_jack_new() function, feel free to ignore that. I also saw now that the same bug is present in hda_jack.c, but I think the other drivers are fine.

How about this approach below? That should also make it possible to use the jack APIs without using a error return.

Arnd

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c index a33234e04d4f..2f72b3c09d92 100644 --- a/sound/pci/hda/hda_jack.c +++ b/sound/pci/hda/hda_jack.c @@ -403,8 +403,10 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,

jack->phantom_jack = !!phantom_jack; jack->type = type; - jack->jack->private_data = jack; - jack->jack->private_free = hda_free_jack_priv; + if (IS_ENABLED(CONFIG_SND_JACK)) { + jack->jack->private_data = jack; + jack->jack->private_free = hda_free_jack_priv; + } state = snd_hda_jack_detect(codec, nid); snd_jack_report(jack->jack, state ? jack->type : 0);

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index f4443b5fbf6e..e9a0f67c92ca 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2532,8 +2532,10 @@ static int add_hdmi_jack_kctl(struct hda_codec *codec, return err;

spec->pcm_rec[pcm_idx].jack = jack; - jack->private_data = &spec->pcm_rec[pcm_idx]; - jack->private_free = free_hdmi_jack_priv; + if (IS_ENABLED(CONFIG_SND_JACK)) { + jack->private_data = &spec->pcm_rec[pcm_idx]; + jack->private_free = free_hdmi_jack_priv; + } return 0; }

Arnd Bergmann

6:09 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tuesday 16 February 2016 17:59:04 Arnd Bergmann wrote:

...

--- a/sound/pci/hda/hda_jack.c +++ b/sound/pci/hda/hda_jack.c @@ -403,8 +403,10 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,

    jack->phantom_jack = !!phantom_jack;
    jack->type = type;

```
  jack->jack->private_data = jack;
```

  jack->jack->private_free = hda_free_jack_priv;

```
  if (IS_ENABLED(CONFIG_SND_JACK)) {
```

          jack->jack->private_data = jack;

          jack->jack->private_free = hda_free_jack_priv;

  }
  state = snd_hda_jack_detect(codec, nid);
  snd_jack_report(jack->jack, state ? jack->type : 0);

Or another idea: if we pass private_{data,free} into snd_jack_new() as arguments, the snd_jack structure can become private to sound/core/jack.c, so we can be sure to never hit this bug again.

Arnd

Takashi Iwai

6:10 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, 16 Feb 2016 17:59:04 +0100, Arnd Bergmann wrote:

...

On Tuesday 16 February 2016 16:38:40 Mark Brown wrote:

...
On Tue, Feb 16, 2016 at 05:18:29PM +0100, Takashi Iwai wrote:

...
Arnd Bergmann wrote:

...
...
Another option might would be to change snd_jack_new() to return an error if that SND_JACK is disabled, and then require all users to handle the error gracefully, i.e. not fail the probe() function but just not use the jack.

...
Yes, I thought of that, too. If select is no good option, it's a good alternative, indeed.

It's going to be a bunch of work to implement though.

I've already sent a v2 to change the snd_jack_new() function, feel free to ignore that. I also saw now that the same bug is present in hda_jack.c, but I think the other drivers are fine.

How about this approach below? That should also make it possible to use the jack APIs without using a error return.

I prefer setting NULL explicitly in snd_jack_new(), and let callers checking NULL. Then we can avoid ugly IS_ENABLE() while the compiler should be still capable to optimize out.

OTOH, I think it'd be a waste of time to bikeshedding too much, so I don't care so much which to take :)

Takashi

--- diff --git a/include/sound/jack.h b/include/sound/jack.h index 23bede121c78..a27c253a3207 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -99,6 +99,7 @@ void snd_jack_report(struct snd_jack *jack, int status); static inline int snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jack, bool initial_kctl, bool phantom_jack) { + *jack = NULL; return 0; }

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c index a33234e04d4f..babd3a8864a1 100644 --- a/sound/pci/hda/hda_jack.c +++ b/sound/pci/hda/hda_jack.c @@ -403,10 +403,12 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,

jack->phantom_jack = !!phantom_jack; jack->type = type; - jack->jack->private_data = jack; - jack->jack->private_free = hda_free_jack_priv; - state = snd_hda_jack_detect(codec, nid); - snd_jack_report(jack->jack, state ? jack->type : 0); + if (jack->jack) { + jack->jack->private_data = jack; + jack->jack->private_free = hda_free_jack_priv; + state = snd_hda_jack_detect(codec, nid); + snd_jack_report(jack->jack, state ? jack->type : 0); + }

return 0; } diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8ee78dbd4c60..34a7b3aaba11 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2158,8 +2158,10 @@ static int add_acomp_jack_kctl(struct hda_codec *codec, if (err < 0) return err; per_pin->acomp_jack = jack; - jack->private_data = per_pin; - jack->private_free = free_acomp_jack_priv; + if (jack) { + jack->private_data = per_pin; + jack->private_free = free_acomp_jack_priv; + } return 0; }

Arnd Bergmann

6:26 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tuesday 16 February 2016 18:10:02 Takashi Iwai wrote:

...

diff --git a/include/sound/jack.h b/include/sound/jack.h index 23bede121c78..a27c253a3207 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -99,6 +99,7 @@ void snd_jack_report(struct snd_jack *jack, int status); static inline int snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jack, bool initial_kctl, bool phantom_jack) {
  *jack = NULL;
  return 0;
}

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c index a33234e04d4f..babd3a8864a1 100644 --- a/sound/pci/hda/hda_jack.c +++ b/sound/pci/hda/hda_jack.c @@ -403,10 +403,12 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
    jack->phantom_jack = !!phantom_jack;
    jack->type = type;
  jack->jack->private_data = jack;
  jack->jack->private_free = hda_free_jack_priv;
  state = snd_hda_jack_detect(codec, nid);
  snd_jack_report(jack->jack, state ? jack->type : 0);
  if (jack->jack) {
          jack->jack->private_data = jack;
          jack->jack->private_free = hda_free_jack_priv;
          state = snd_hda_jack_detect(codec, nid);
          snd_jack_report(jack->jack, state ? jack->type : 0);
  }

  return 0;
} diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8ee78dbd4c60..34a7b3aaba11 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2158,8 +2158,10 @@ static int add_acomp_jack_kctl(struct hda_codec *codec, if (err < 0) return err; per_pin->acomp_jack = jack;
  jack->private_data = per_pin;
  jack->private_free = free_acomp_jack_priv;
  if (jack) {
          jack->private_data = per_pin;
          jack->private_free = free_acomp_jack_priv;
  }
  return 0;
}

Looks good to me.

Acked-by: Arnd Bergmann arnd@arndb.de

Arnd Bergmann

11:08 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tuesday 16 February 2016 18:26:09 Arnd Bergmann wrote:

...

            return err;
    per_pin->acomp_jack = jack;

```
  jack->private_data = per_pin;
```

  jack->private_free = free_acomp_jack_priv;

```
  if (jack) {
```

          jack->private_data = per_pin;

          jack->private_free = free_acomp_jack_priv;

```
  }
  return 0;
```

}

Looks good to me.

Acked-by: Arnd Bergmann arnd@arndb.de

Actually, on second thought, there is one notable downside of this approach: 'jack' is guaranteed to be NULL here, so gcc won't produce a warning if another driver makes the same mistake:

By default, we only get a warning for uses of uninitialized variables, but not those that are known to be NULL. I think some compiler warnings are able to warn about that too, but I forgot the details.

Arnd

Takashi Iwai

17 Feb 17 Feb

10:03 a.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Tue, 16 Feb 2016 23:08:13 +0100, Arnd Bergmann wrote:

...

On Tuesday 16 February 2016 18:26:09 Arnd Bergmann wrote:

...
...
            return err;
    per_pin->acomp_jack = jack;
  jack->private_data = per_pin;
  jack->private_free = free_acomp_jack_priv;
  if (jack) {
          jack->private_data = per_pin;
          jack->private_free = free_acomp_jack_priv;
  }
  return 0;
}
Looks good to me.

Acked-by: Arnd Bergmann arnd@arndb.de
Actually, on second thought, there is one notable downside of this approach: 'jack' is guaranteed to be NULL here, so gcc won't produce a warning if another driver makes the same mistake:

By default, we only get a warning for uses of uninitialized variables, but not those that are known to be NULL. I think some compiler warnings are able to warn about that too, but I forgot the details.

Yes, that is a good point.

On third thought, we should look back how this issue came out.

Originally HD-audio driver had another jack implementation using ctl API, and this could be unconditionally enabled. The kctl jack was unified into the input jack layer later, and the dependency issue came out. In that sense, the problem is that SND_JACK dependency is still present although it's not needed -- namely, the jack layer itself can be implemented in kctl even without input devices.

So, the missing piece is the conditional build of jack.c & co without CONFIG_INPUT. Then the dependency in the user's side can be ignored.

Below is the patch to do that.

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: jack: Allow building the jack layer without input device

Since the recent integration of kctl jack and input jack layers, we can basically build the jack layer even without input devices. That is, the jack layer itself can be built with conditional to enable the input device support or not, while the users may enable always CONFIG_SND_JACK unconditionally.

For achieving it, this patch changes the following: - A new Kconfig, CONFIG_SND_JACK_INPUT_DEV, was introduced to indicate whether the jack layer supports the input device, - A few items in snd_jack struct and relevant codes are conditionally built upon CONFIG_SND_JACK_INPUT_DEV, - The users of CONFIG_SND_JACK drop the messy dependency on CONFIG_INPUT, but just select SND_JACK.

This change also automagically fixes a potential bug in HD-audio driver Arnd reported, where the NULL or uninitialized jack instance is dereferenced.

Reported-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/jack.h | 23 ++++++++++++++--------- sound/core/Kconfig | 9 ++++++--- sound/core/jack.c | 23 ++++++++++++++++++++--- sound/pci/Kconfig | 2 +- sound/pci/hda/Kconfig | 2 +- sound/soc/Kconfig | 2 +- 6 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h index 23bede121c78..1e84bfb553cf 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -72,14 +72,16 @@ enum snd_jack_types { #define SND_JACK_SWITCH_TYPES 6

struct snd_jack { - struct input_dev *input_dev; struct list_head kctl_list; struct snd_card *card; + const char *id; +#ifdef CONFIG_SND_JACK_INPUT_DEV + struct input_dev *input_dev; int registered; int type; - const char *id; char name[100]; unsigned int key[6]; /* Keep in sync with definitions above */ +#endif /* CONFIG_SND_JACK_INPUT_DEV */ void *private_data; void (*private_free)(struct snd_jack *); }; @@ -89,10 +91,11 @@ struct snd_jack { int snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jack, bool initial_kctl, bool phantom_jack); int snd_jack_add_new_kctl(struct snd_jack *jack, const char * name, int mask); +#ifdef CONFIG_SND_JACK_INPUT_DEV void snd_jack_set_parent(struct snd_jack *jack, struct device *parent); int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type, int keytype); - +#endif void snd_jack_report(struct snd_jack *jack, int status);

#else @@ -107,6 +110,13 @@ static inline int snd_jack_add_new_kctl(struct snd_jack *jack, const char * name return 0; }

+static inline void snd_jack_report(struct snd_jack *jack, int status) +{ +} + +#endif + +#if !defined(CONFIG_SND_JACK) || !defined(CONFIG_SND_JACK_INPUT_DEV) static inline void snd_jack_set_parent(struct snd_jack *jack, struct device *parent) { @@ -118,11 +128,6 @@ static inline int snd_jack_set_key(struct snd_jack *jack, { return 0; } - -static inline void snd_jack_report(struct snd_jack *jack, int status) -{ -} - -#endif +#endif /* !CONFIG_SND_JACK || !CONFIG_SND_JACK_INPUT_DEV */

#endif diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a2a1e24becc6..8b71e15898cd 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -24,11 +24,14 @@ config SND_RAWMIDI config SND_COMPRESS_OFFLOAD tristate

-# To be effective this also requires INPUT - users should say: -# select SND_JACK if INPUT=y || INPUT=SND -# to avoid having to force INPUT on. config SND_JACK + tristate + +# enable input device support in jack layer +config SND_JACK_INPUT_DEV bool + depends on SND_JACK + default y if INPUT=y || INPUT=SND

config SND_SEQUENCER tristate "Sequencer support" diff --git a/sound/core/jack.c b/sound/core/jack.c index 7237acbdcbbc..f652e90efd7e 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -32,6 +32,7 @@ struct snd_jack_kctl { unsigned int mask_bits; /* only masked status bits are reported via kctl */ };

+#ifdef CONFIG_SND_JACK_INPUT_DEV static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { SW_HEADPHONE_INSERT, SW_MICROPHONE_INSERT, @@ -40,9 +41,11 @@ static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { SW_VIDEOOUT_INSERT, SW_LINEIN_INSERT, }; +#endif /* CONFIG_SND_JACK_INPUT_DEV */

static int snd_jack_dev_disconnect(struct snd_device *device) { +#ifdef CONFIG_SND_JACK_INPUT_DEV struct snd_jack *jack = device->device_data;

if (!jack->input_dev) @@ -55,6 +58,7 @@ static int snd_jack_dev_disconnect(struct snd_device *device) else input_free_device(jack->input_dev); jack->input_dev = NULL; +#endif /* CONFIG_SND_JACK_INPUT_DEV */ return 0; }

@@ -79,6 +83,7 @@ static int snd_jack_dev_free(struct snd_device *device) return 0; }

+#ifdef CONFIG_SND_JACK_INPUT_DEV static int snd_jack_dev_register(struct snd_device *device) { struct snd_jack *jack = device->device_data; @@ -116,6 +121,7 @@ static int snd_jack_dev_register(struct snd_device *device)

return err; } +#endif /* CONFIG_SND_JACK_INPUT_DEV */

static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl) { @@ -209,11 +215,12 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack *jack; struct snd_jack_kctl *jack_kctl = NULL; int err; - int i; static struct snd_device_ops ops = { .dev_free = snd_jack_dev_free, +#ifdef CONFIG_SND_JACK_INPUT_DEV .dev_register = snd_jack_dev_register, .dev_disconnect = snd_jack_dev_disconnect, +#endif /* CONFIG_SND_JACK_INPUT_DEV */ };

if (initial_kctl) { @@ -230,6 +237,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,

/* don't creat input device for phantom jack */ if (!phantom_jack) { +#ifdef CONFIG_SND_JACK_INPUT_DEV + int i; + jack->input_dev = input_allocate_device(); if (jack->input_dev == NULL) { err = -ENOMEM; @@ -245,6 +255,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, input_set_capability(jack->input_dev, EV_SW, jack_switch_types[i]);

+#endif /* CONFIG_SND_JACK_INPUT_DEV */ }

err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops); @@ -262,13 +273,16 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, return 0;

fail_input: +#ifdef CONFIG_SND_JACK_INPUT_DEV input_free_device(jack->input_dev); +#endif kfree(jack->id); kfree(jack); return err; } EXPORT_SYMBOL(snd_jack_new);

+#ifdef CONFIG_SND_JACK_INPUT_DEV /** * snd_jack_set_parent - Set the parent device for a jack * @@ -326,10 +340,10 @@ int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,

jack->type |= type; jack->key[key] = keytype; - return 0; } EXPORT_SYMBOL(snd_jack_set_key); +#endif /* CONFIG_SND_JACK_INPUT_DEV */

/** * snd_jack_report - Report the current status of a jack @@ -340,7 +354,9 @@ EXPORT_SYMBOL(snd_jack_set_key); void snd_jack_report(struct snd_jack *jack, int status) { struct snd_jack_kctl *jack_kctl; +#ifdef CONFIG_SND_JACK_INPUT_DEV int i; +#endif

if (!jack) return; @@ -349,6 +365,7 @@ void snd_jack_report(struct snd_jack *jack, int status) snd_kctl_jack_report(jack->card, jack_kctl->kctl, status & jack_kctl->mask_bits);

+#ifdef CONFIG_SND_JACK_INPUT_DEV if (!jack->input_dev) return;

@@ -369,6 +386,6 @@ void snd_jack_report(struct snd_jack *jack, int status) }

input_sync(jack->input_dev); - +#endif /* CONFIG_SND_JACK_INPUT_DEV */ } EXPORT_SYMBOL(snd_jack_report); diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig index 8f6594a7d37f..32151d8c6bb8 100644 --- a/sound/pci/Kconfig +++ b/sound/pci/Kconfig @@ -866,7 +866,7 @@ config SND_VIRTUOSO select SND_OXYGEN_LIB select SND_PCM select SND_MPU401_UART - select SND_JACK if INPUT=y || INPUT=SND + select SND_JACK help Say Y here to include support for sound cards based on the Asus AV66/AV100/AV200 chips, i.e., Xonar D1, DX, D2, D2X, DS, DSX, diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index e94cfd5c69f7..bb02c2d48fd5 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -4,7 +4,7 @@ config SND_HDA tristate select SND_PCM select SND_VMASTER - select SND_JACK if INPUT=y || INPUT=SND + select SND_JACK select SND_HDA_CORE

-- 2.7.1

Takashi Iwai

10:35 a.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Wed, 17 Feb 2016 10:03:50 +0100, Takashi Iwai wrote:

...

--- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -24,11 +24,14 @@ config SND_RAWMIDI config SND_COMPRESS_OFFLOAD tristate

-# To be effective this also requires INPUT - users should say: -# select SND_JACK if INPUT=y || INPUT=SND -# to avoid having to force INPUT on. config SND_JACK

tristate

Oops, some unintentional change slipped in. This must be bool.

Below is the revised patch.

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: jack: Allow building the jack layer without input device

This change also automagically fixes a potential bug in HD-audio driver Arnd reported, where the NULL or uninitialized jack instance is dereferenced.

#else @@ -107,6 +110,13 @@ static inline int snd_jack_add_new_kctl(struct snd_jack *jack, const char * name return 0; }

#endif diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a2a1e24becc6..6d12ca9bcb80 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -24,12 +24,15 @@ config SND_RAWMIDI config SND_COMPRESS_OFFLOAD tristate

-# To be effective this also requires INPUT - users should say: -# select SND_JACK if INPUT=y || INPUT=SND -# to avoid having to force INPUT on. config SND_JACK bool

+# enable input device support in jack layer +config SND_JACK_INPUT_DEV + bool + depends on SND_JACK + default y if INPUT=y || INPUT=SND + config SND_SEQUENCER tristate "Sequencer support" select SND_TIMER diff --git a/sound/core/jack.c b/sound/core/jack.c index 7237acbdcbbc..f652e90efd7e 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -32,6 +32,7 @@ struct snd_jack_kctl { unsigned int mask_bits; /* only masked status bits are reported via kctl */ };

static int snd_jack_dev_disconnect(struct snd_device *device) { +#ifdef CONFIG_SND_JACK_INPUT_DEV struct snd_jack *jack = device->device_data;

@@ -79,6 +83,7 @@ static int snd_jack_dev_free(struct snd_device *device) return 0; }

return err; } +#endif /* CONFIG_SND_JACK_INPUT_DEV */

if (initial_kctl) { @@ -230,6 +237,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,

+#endif /* CONFIG_SND_JACK_INPUT_DEV */ }

err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops); @@ -262,13 +273,16 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, return 0;

fail_input: +#ifdef CONFIG_SND_JACK_INPUT_DEV input_free_device(jack->input_dev); +#endif kfree(jack->id); kfree(jack); return err; } EXPORT_SYMBOL(snd_jack_new);

+#ifdef CONFIG_SND_JACK_INPUT_DEV /** * snd_jack_set_parent - Set the parent device for a jack * @@ -326,10 +340,10 @@ int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,

jack->type |= type; jack->key[key] = keytype; - return 0; } EXPORT_SYMBOL(snd_jack_set_key); +#endif /* CONFIG_SND_JACK_INPUT_DEV */

if (!jack) return; @@ -349,6 +365,7 @@ void snd_jack_report(struct snd_jack *jack, int status) snd_kctl_jack_report(jack->card, jack_kctl->kctl, status & jack_kctl->mask_bits);

+#ifdef CONFIG_SND_JACK_INPUT_DEV if (!jack->input_dev) return;

@@ -369,6 +386,6 @@ void snd_jack_report(struct snd_jack *jack, int status) }

-- 2.7.1

Arnd Bergmann

10:40 a.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Wednesday 17 February 2016 10:35:40 Takashi Iwai wrote:

...

From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: jack: Allow building the jack layer without input device

Since the recent integration of kctl jack and input jack layers, we can basically build the jack layer even without input devices. That is, the jack layer itself can be built with conditional to enable the input device support or not, while the users may enable always CONFIG_SND_JACK unconditionally.

For achieving it, this patch changes the following:

A new Kconfig, CONFIG_SND_JACK_INPUT_DEV, was introduced to indicate whether the jack layer supports the input device,

A few items in snd_jack struct and relevant codes are conditionally built upon CONFIG_SND_JACK_INPUT_DEV,

The users of CONFIG_SND_JACK drop the messy dependency on CONFIG_INPUT.

This change also automagically fixes a potential bug in HD-audio driver Arnd reported, where the NULL or uninitialized jack instance is dereferenced.

Reported-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Takashi Iwai tiwai@suse.de

Looks great, that indeed takes care of the issue I reported and makes the code more useful and more robust overall.

Acked-by: Arnd Bergmann arnd@arndb.de

Arnd Bergmann

24 Feb 24 Feb

5:18 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Wednesday 17 February 2016 10:35:40 Takashi Iwai wrote:

...

On Wed, 17 Feb 2016 10:03:50 +0100,

const char *id;

+#ifdef CONFIG_SND_JACK_INPUT_DEV

struct input_dev *input_dev; int registered; int type;

const char *id; char name[100]; unsigned int key[6]; /* Keep in sync with definitions above */

+#endif /* CONFIG_SND_JACK_INPUT_DEV */ void *private_data; void (*private_free)(struct snd_jack *); };

I got a build error from this today, as the trace event tries to print the jack "name" field. I've managed to get it to build again by printing the "id" field in place of the "name". The name is normally assigned from id in snd_jack_dev_register using

snprintf(jack->name, sizeof(jack->name), "%s %s", card->shortname, jack->id);

but that code is not called here at all. My patch will slightly alter the output as a consequence, but I don't know if this change is critical or not.

Arnd

diff --git a/include/trace/events/asoc.h b/include/trace/events/asoc.h index 317a1ed2f4ac..9130dd5a184a 100644 --- a/include/trace/events/asoc.h +++ b/include/trace/events/asoc.h @@ -231,13 +231,13 @@ TRACE_EVENT(snd_soc_jack_report, TP_ARGS(jack, mask, val),

TP_STRUCT__entry( - __string( name, jack->jack->name ) + __string( name, jack->jack->id ) __field( int, mask ) __field( int, val ) ),

TP_fast_assign( - __assign_str(name, jack->jack->name); + __assign_str(name, jack->jack->id); __entry->mask = mask; __entry->val = val; ), @@ -253,12 +253,12 @@ TRACE_EVENT(snd_soc_jack_notify, TP_ARGS(jack, val),

TP_STRUCT__entry( - __string( name, jack->jack->name ) + __string( name, jack->jack->id ) __field( int, val ) ),

TP_fast_assign( - __assign_str(name, jack->jack->name); + __assign_str(name, jack->jack->id); __entry->val = val; ),

Takashi Iwai

5:25 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Wed, 24 Feb 2016 17:18:58 +0100, Arnd Bergmann wrote:

...

On Wednesday 17 February 2016 10:35:40 Takashi Iwai wrote:

...
On Wed, 17 Feb 2016 10:03:50 +0100,

const char *id;

+#ifdef CONFIG_SND_JACK_INPUT_DEV

struct input_dev *input_dev; int registered; int type;

const char *id; char name[100]; unsigned int key[6]; /* Keep in sync with definitions above */

+#endif /* CONFIG_SND_JACK_INPUT_DEV */ void *private_data; void (*private_free)(struct snd_jack *); };

I got a build error from this today, as the trace event tries to print the jack "name" field. I've managed to get it to build again by printing the "id" field in place of the "name". The name is normally assigned from id in snd_jack_dev_register using
    snprintf(jack->name, sizeof(jack->name), "%s %s",
             card->shortname, jack->id);
but that code is not called here at all. My patch will slightly alter the output as a consequence, but I don't know if this change is critical or not.

Thanks for catching this. Yes, your fix is correct. This must have been a wrong pick up when converting from the standalone hda jack to unified jack stuff.

Could you send a proper patch for inclusion?

Takashi

...

Arnd

diff --git a/include/trace/events/asoc.h b/include/trace/events/asoc.h index 317a1ed2f4ac..9130dd5a184a 100644 --- a/include/trace/events/asoc.h +++ b/include/trace/events/asoc.h @@ -231,13 +231,13 @@ TRACE_EVENT(snd_soc_jack_report, TP_ARGS(jack, mask, val),

TP_STRUCT__entry(
__string(	name,		jack->jack->name	)
__string(	name,		jack->jack->id		)
__field( int, mask ) __field( int, val ) ),

TP_fast_assign(
__assign_str(name, jack->jack->name);
__assign_str(name, jack->jack->id);
__entry->mask = mask; __entry->val = val; ),
@@ -253,12 +253,12 @@ TRACE_EVENT(snd_soc_jack_notify, TP_ARGS(jack, val),

TP_STRUCT__entry(
__string(	name,		jack->jack->name	)
__string(	name,		jack->jack->id		)
__field( int, val ) ),

TP_fast_assign(
__assign_str(name, jack->jack->name);
__assign_str(name, jack->jack->id);
__entry->val = val; ),

Arnd Bergmann

5:39 p.m.

New subject: [alsa-devel] [PATCH] sound: hdmi: avoid dereferencing uninitialized 'jack' pointer

On Wednesday 24 February 2016 17:25:42 Takashi Iwai wrote:

...

Thanks for catching this. Yes, your fix is correct. This must have been a wrong pick up when converting from the standalone hda jack to unified jack stuff.

Could you send a proper patch for inclusion?

Ok, done.

Arnd

Mark Brown

26 Feb 26 Feb

3:46 a.m.

New subject: [alsa-devel] Applied "ASoC: trace: fix printing jack name" to the asoc tree

The patch

ASoC: trace: fix printing jack name

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks, Mark

...

From f4833a519aec793cf8349bf479589d37473ef6a7 Mon Sep 17 00:00:00 2001

From: Arnd Bergmann arnd@arndb.de Date: Wed, 24 Feb 2016 17:38:14 +0100 Subject: [PATCH] ASoC: trace: fix printing jack name

After a change to the snd_jack structure, the 'name' member is no longer available in all configurations, which results in a build failure in the tracing code:

include/trace/events/asoc.h: In function 'trace_event_raw_event_snd_soc_jack_report': include/trace/events/asoc.h:240:32: error: 'struct snd_jack' has no member named 'name'

The name field is normally initialized from the card shortname and the jack "id" field:

snprintf(jack->name, sizeof(jack->name), "%s %s", card->shortname, jack->id);

This changes the tracing output to just contain the 'id' by itself, which slightly changes the output format but avoids the link error and is hopefully still enough to see what is going on.

Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: fe0d128c57bf ("ALSA: jack: Allow building the jack layer without input device") Signed-off-by: Mark Brown broonie@kernel.org --- include/trace/events/asoc.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

TP_STRUCT__entry( - __string( name, jack->jack->name ) + __string( name, jack->jack->id ) __field( int, mask ) __field( int, val ) ),

TP_STRUCT__entry( - __string( name, jack->jack->name ) + __string( name, jack->jack->id ) __field( int, val ) ),

TP_fast_assign( - __assign_str(name, jack->jack->name); + __assign_str(name, jack->jack->id); __entry->val = val; ),

-- 2.7.0

3222

Age (days ago)

3232

Last active (days ago)

List overview

Download

17 comments

3 participants

tags (0)

participants (3)

Arnd Bergmann
Mark Brown
Takashi Iwai