Iwai-san,

On Oct 25 2014 20:40, Christian Vogel wrote:

snd_bebob_stream_check_internal_clock() may get an id from saffirepro_both_clk_src_get (via clk_src->get()) that was uninitialized.

a) make logic in saffirepro_both_clk_src_get explicit b) test if id used in snd_bebob_stream_check_internal_clock matches array size

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp Signed-off-by: Christian Vogel vogelchr@vogel.cx

---

sound/firewire/bebob/bebob_focusrite.c | 62 ++++++++++++++++++++++++++-------- sound/firewire/bebob/bebob_stream.c | 18 ++++++++-- 2 files changed, 63 insertions(+), 17 deletions(-)

[alsa-devel] [PATCH] Uninitialized id returned by saffirepro_both_clk_src_get. http://mailman.alsa-project.org/pipermail/alsa-devel/2014-October/082810.htm...

Would you please apply this patch to 'for-linus' branch and C.C.ed to Linux stable branch, as a bug fix?

The subject should be with a prefix 'ALSA: bebob: ', and I hope to add 'Reviewed-by' instead of 'Signed-off-by' for myself.

Regards

Takashi Sakamoto o-takashi@sakamocchi.jp

diff --git a/sound/firewire/bebob/bebob_focusrite.c b/sound/firewire/bebob/bebob_focusrite.c index 45a0eed..7b18e84 100644 --- a/sound/firewire/bebob/bebob_focusrite.c +++ b/sound/firewire/bebob/bebob_focusrite.c @@ -27,12 +27,14 @@ #define SAFFIRE_CLOCK_SOURCE_INTERNAL 0 #define SAFFIRE_CLOCK_SOURCE_SPDIF 1

-/* '1' is absent, why... */ +/* clock sources as returned from register of Saffire Pro 10 and 26 */ #define SAFFIREPRO_CLOCK_SOURCE_INTERNAL 0 +#define SAFFIREPRO_CLOCK_SOURCE_SKIP 1 /* never used on hardware */ #define SAFFIREPRO_CLOCK_SOURCE_SPDIF 2 -#define SAFFIREPRO_CLOCK_SOURCE_ADAT1 3 -#define SAFFIREPRO_CLOCK_SOURCE_ADAT2 4 +#define SAFFIREPRO_CLOCK_SOURCE_ADAT1 3 /* not used on s.pro. 10 */ +#define SAFFIREPRO_CLOCK_SOURCE_ADAT2 4 /* not used on s.pro. 10 */ #define SAFFIREPRO_CLOCK_SOURCE_WORDCLOCK 5 +#define SAFFIREPRO_CLOCK_SOURCE_COUNT 6

/* S/PDIF, ADAT1, ADAT2 is enabled or not. three quadlets */ #define SAFFIREPRO_ENABLE_DIG_IFACES 0x01a4 @@ -101,13 +103,34 @@ saffire_write_quad(struct snd_bebob *bebob, u64 offset, u32 value) &data, sizeof(__be32), 0); }

+static char *const saffirepro_10_clk_src_labels[] = {

• SND_BEBOB_CLOCK_INTERNAL, "S/PDIF", "Word Clock"

+}; static char *const saffirepro_26_clk_src_labels[] = { SND_BEBOB_CLOCK_INTERNAL, "S/PDIF", "ADAT1", "ADAT2", "Word Clock" };

-static char *const saffirepro_10_clk_src_labels[] = {

• SND_BEBOB_CLOCK_INTERNAL, "S/PDIF", "Word Clock"

+/* Value maps between registers and labels for SaffirePro 10/26. */ +static const char saffirepro_clk_maps[][SAFFIREPRO_CLOCK_SOURCE_COUNT] = {

• /* SaffirePro 10 */
• [0] = {

• [SAFFIREPRO_CLOCK_SOURCE_INTERNAL]  =  0,
  

• [SAFFIREPRO_CLOCK_SOURCE_SKIP]      = -1, /* not supported */
  

• [SAFFIREPRO_CLOCK_SOURCE_SPDIF]     =  1,
  

• [SAFFIREPRO_CLOCK_SOURCE_ADAT1]     = -1, /* not supported */
  

• [SAFFIREPRO_CLOCK_SOURCE_ADAT2]     = -1, /* not supported */
  

• [SAFFIREPRO_CLOCK_SOURCE_WORDCLOCK] =  2,
  

• },
• /* SaffirePro 26 */
• [1] = {

• [SAFFIREPRO_CLOCK_SOURCE_INTERNAL]  =  0,
  

• [SAFFIREPRO_CLOCK_SOURCE_SKIP]      = -1, /* not supported */
  

• [SAFFIREPRO_CLOCK_SOURCE_SPDIF]     =  1,
  

• [SAFFIREPRO_CLOCK_SOURCE_ADAT1]     =  2,
  

• [SAFFIREPRO_CLOCK_SOURCE_ADAT2]     =  3,
  

• [SAFFIREPRO_CLOCK_SOURCE_WORDCLOCK] =  4,
  

• }

};

static int saffirepro_both_clk_freq_get(struct snd_bebob *bebob, unsigned int *rate) { @@ -138,24 +161,35 @@ saffirepro_both_clk_freq_set(struct snd_bebob *bebob, unsigned int rate)

return saffire_write_quad(bebob, SAFFIREPRO_RATE_NOREBOOT, id); }

+/*

• • query hardware for current clock source, return our internally
• • used clock index in *id, depending on hardware.
• */

static int saffirepro_both_clk_src_get(struct snd_bebob *bebob, unsigned int *id) { int err;

• u32 value;

• u32 value; /* clock source read from hw register */

• const char *map;

  err = saffire_read_quad(bebob, SAFFIREPRO_OFFSET_CLOCK_SOURCE, &value); if (err < 0) goto end;

• if (bebob->spec->clock->labels == saffirepro_10_clk_src_labels) {

• if (value == SAFFIREPRO_CLOCK_SOURCE_WORDCLOCK)
  

• 	*id = 2;
  

• else if (value == SAFFIREPRO_CLOCK_SOURCE_SPDIF)
  

• 	*id = 1;
  

• } else if (value > 1) {

• *id = value - 1;
  

• /* depending on hardware, use a different mapping */
• if (bebob->spec->clock->labels == saffirepro_10_clk_src_labels)

• map = saffirepro_clk_maps[0];
  

• else

• map = saffirepro_clk_maps[1];
  

• /* In a case that this driver cannot handle the value of register. */
• if (value >= SAFFIREPRO_CLOCK_SOURCE_COUNT || map[value] < 0) {

• err = -EIO;
  

• goto end;
  

  }
• *id = (unsigned int)map[value];

end: return err; } diff --git a/sound/firewire/bebob/bebob_stream.c b/sound/firewire/bebob/bebob_stream.c index ef4d0c9..1aab0a3 100644 --- a/sound/firewire/bebob/bebob_stream.c +++ b/sound/firewire/bebob/bebob_stream.c @@ -129,12 +129,24 @@ snd_bebob_stream_check_internal_clock(struct snd_bebob *bebob, bool *internal) /* 1.The device has its own operation to switch source of clock */ if (clk_spec) { err = clk_spec->get(bebob, &id);

• if (err < 0)
  

• if (err < 0) {
  dev_err(&bebob->unit->device,
  	"fail to get clock source: %d\n", err);
  

• else if (strncmp(clk_spec->labels[id], SND_BEBOB_CLOCK_INTERNAL,
  

• 		 strlen(SND_BEBOB_CLOCK_INTERNAL)) == 0)
  

• 	goto end;
  

• }
  

• if (id >= clk_spec->num) {
  

• 	dev_err(&bebob->unit->device,
  

• 		"clock source %d out of range 0..%d\n",
  

• 		id, clk_spec->num - 1);
  

• 	err = -EIO;
  

• 	goto end;
  

• }
  

• if (strncmp(clk_spec->labels[id], SND_BEBOB_CLOCK_INTERNAL,
  

• 	    strlen(SND_BEBOB_CLOCK_INTERNAL)) == 0)
  *internal = true;
  

• goto end; }