| Message ID | 1414237241-18984-2-git-send-email-vogelchr@vogel.cx (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.html 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; > } > >
At Mon, 27 Oct 2014 21:55:48 +0900, Takashi Sakamoto wrote: > > 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.html > > Would you please apply this patch to 'for-linus' branch and C.C.ed to > Linux stable branch, as a bug fix? OK. But we need a fix in addition. See below. > The subject should be with a prefix 'ALSA: bebob: ', and I hope to add > 'Reviewed-by' instead of 'Signed-off-by' for myself. If the patch doesn't come from you, signed-off-by is actually a wrong tag. I'll replace it accordingly. About the patch: > > +/* 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 */ If you need to handle a negative value, you must specify "signed char". Without "signed" prefix, it might be unsigned, depending on the architecture, in the case of char. > > 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; Ditto. thanks, Takashi
Hi, On Oct 27 2014 22 13 Takashi Iwai wrote: > About the patch: >>> +/* 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 */ > > If you need to handle a negative value, you must specify "signed > char". Without "signed" prefix, it might be unsigned, depending on > the architecture, in the case of char. > >>> 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; > > Ditto. Yep, exactly. I missed them... Thanks for your indication and applying to the branch, C.C.ed to stable: http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?h=for-linus&id=d1d0b6b668818571122d30d68a0b3f768bd83a52 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; }