| Message ID | 1453441612-29902-1-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Jan 2016 06:46:52 +0100, Vinod Koul wrote: > > TLV byte control are new type of byte controls added in kernel where > controls can have large sizes. > > For these controls querying with 4096 size fails, so use the queried size to > read the control > > This fixes the crash with current cget/contents on these type of controls Hmm... Theoretically the TLV size and the element size are independent. So, it's not good to check only the type being SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, too. Basically it is an abuse in ASoC side to return the size of TLV by the element's info. Usually such value would lead to crash, but the unique point of ASoC ext bytes ctl is that it doesn't have RW accesses but only TLV_RW accesses. So, instead of only checking the type being BYTES, check the accesses. Only when all these conditions are met, we may take the count as TLV (element) size. (And still we should have the sanity check of the value, too.) Yet, this isn't a really "fix" for the crash. Even without the patch it shouldn't crash -- it should receive 4096 bytes, and tries to decode. Where did you get the crash exactly? thanks, Takashi > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > amixer/amixer.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/amixer/amixer.c b/amixer/amixer.c > index db1849333da3..cfe13592347f 100644 > --- a/amixer/amixer.c > +++ b/amixer/amixer.c > @@ -588,7 +588,7 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, > int level) > { > int err; > - unsigned int item, idx, count, *tlv; > + unsigned int item, idx, count, *tlv, tlv_sz; > snd_ctl_elem_type_t type; > snd_ctl_elem_id_t *id; > snd_ctl_elem_info_t *info; > @@ -682,13 +682,20 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, > __skip_read: > if (!snd_ctl_elem_info_is_tlv_readable(info)) > goto __skip_tlv; > - tlv = malloc(4096); > - if ((err = snd_hctl_elem_tlv_read(elem, tlv, 4096)) < 0) { > + > + if (type == SND_CTL_ELEM_TYPE_BYTES) > + tlv_sz = count + 2 * sizeof(unsigned int); > + else > + tlv_sz = 4096; > + > + tlv = malloc(tlv_sz); > + > + if ((err = snd_hctl_elem_tlv_read(elem, tlv, tlv_sz)) < 0) { > error("Control %s element TLV read error: %s\n", card, snd_strerror(err)); > free(tlv); > return err; > } > - decode_tlv(strlen(space), tlv, 4096); > + decode_tlv(strlen(space), tlv, tlv_sz); > free(tlv); > } > __skip_tlv: > -- > 1.9.1 >
On Fri, Jan 22, 2016 at 07:19:10AM +0100, Takashi Iwai wrote: > On Fri, 22 Jan 2016 06:46:52 +0100, > Vinod Koul wrote: > > > > TLV byte control are new type of byte controls added in kernel where > > controls can have large sizes. > > > > For these controls querying with 4096 size fails, so use the queried size to > > read the control > > > > This fixes the crash with current cget/contents on these type of controls > > Hmm... Theoretically the TLV size and the element size are > independent. So, it's not good to check only the type being > SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, > too. > > Basically it is an abuse in ASoC side to return the size of > TLV by the element's info. Usually such value would lead to crash, > but the unique point of ASoC ext bytes ctl is that it doesn't have RW > accesses but only TLV_RW accesses. But isn't that already checked? Since we are in the code which will be executed only for tlv as snd_ctl_elem_info_is_tlv_readable() ensures that. So this is only for tlv + bytes ... > > So, instead of only checking the type being BYTES, check the > accesses. Only when all these conditions are met, we may take the > count as TLV (element) size. (And still we should have the sanity > check of the value, too.) > > Yet, this isn't a really "fix" for the crash. Even without the patch > it shouldn't crash -- it should receive 4096 bytes, and tries to > decode. Where did you get the crash exactly? in alsa-lib snd_ctl_hw_elem_tlv() when it tries to do memcpy for tlv read. But the crash is caused as tlv read is large and we have only 4096 size buffer, so we ensure we give right size buffer here
On Fri, 22 Jan 2016 08:46:23 +0100, Vinod Koul wrote: > > On Fri, Jan 22, 2016 at 07:19:10AM +0100, Takashi Iwai wrote: > > On Fri, 22 Jan 2016 06:46:52 +0100, > > Vinod Koul wrote: > > > > > > TLV byte control are new type of byte controls added in kernel where > > > controls can have large sizes. > > > > > > For these controls querying with 4096 size fails, so use the queried size to > > > read the control > > > > > > This fixes the crash with current cget/contents on these type of controls > > > > Hmm... Theoretically the TLV size and the element size are > > independent. So, it's not good to check only the type being > > SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, > > too. > > > > Basically it is an abuse in ASoC side to return the size of > > TLV by the element's info. Usually such value would lead to crash, > > but the unique point of ASoC ext bytes ctl is that it doesn't have RW > > accesses but only TLV_RW accesses. > > But isn't that already checked? Since we are in the code which will be > executed only for tlv as snd_ctl_elem_info_is_tlv_readable() ensures that. > So this is only for tlv + bytes ... I meant setting a bogus count value in info would usually lead to a crash. The point isn't about TLV access. > > So, instead of only checking the type being BYTES, check the > > accesses. Only when all these conditions are met, we may take the > > count as TLV (element) size. (And still we should have the sanity > > check of the value, too.) > > > > Yet, this isn't a really "fix" for the crash. Even without the patch > > it shouldn't crash -- it should receive 4096 bytes, and tries to > > decode. Where did you get the crash exactly? > > in alsa-lib snd_ctl_hw_elem_tlv() when it tries to do memcpy for tlv read. > But the crash is caused as tlv read is large and we have only 4096 size > buffer, Hm, it has a check static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, unsigned int numid, unsigned int *tlv, unsigned int tlv_size) { .... if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { free(xtlv); return -EFAULT; } memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); Do you mean somewhere here triggers a crash? > so we ensure we give right size buffer here Actually we should have an API function that returns the size of TLV. Then amixer can adjust the allocation size. Takashi > > -- > ~Vinod > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > --- > > > amixer/amixer.c | 15 +++++++++++---- > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/amixer/amixer.c b/amixer/amixer.c > > > index db1849333da3..cfe13592347f 100644 > > > --- a/amixer/amixer.c > > > +++ b/amixer/amixer.c > > > @@ -588,7 +588,7 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, > > > int level) > > > { > > > int err; > > > - unsigned int item, idx, count, *tlv; > > > + unsigned int item, idx, count, *tlv, tlv_sz; > > > snd_ctl_elem_type_t type; > > > snd_ctl_elem_id_t *id; > > > snd_ctl_elem_info_t *info; > > > @@ -682,13 +682,20 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, > > > __skip_read: > > > if (!snd_ctl_elem_info_is_tlv_readable(info)) > > > goto __skip_tlv; > > > - tlv = malloc(4096); > > > - if ((err = snd_hctl_elem_tlv_read(elem, tlv, 4096)) < 0) { > > > + > > > + if (type == SND_CTL_ELEM_TYPE_BYTES) > > > + tlv_sz = count + 2 * sizeof(unsigned int); > > > + else > > > + tlv_sz = 4096; > > > + > > > + tlv = malloc(tlv_sz); > > > + > > > + if ((err = snd_hctl_elem_tlv_read(elem, tlv, tlv_sz)) < 0) { > > > error("Control %s element TLV read error: %s\n", card, snd_strerror(err)); > > > free(tlv); > > > return err; > > > } > > > - decode_tlv(strlen(space), tlv, 4096); > > > + decode_tlv(strlen(space), tlv, tlv_sz); > > > free(tlv); > > > } > > > __skip_tlv: > > > -- > > > 1.9.1 > > > >
On Fri, 22 Jan 2016 09:09:13 +0100, Takashi Iwai wrote: > > On Fri, 22 Jan 2016 08:46:23 +0100, > Vinod Koul wrote: > > > > On Fri, Jan 22, 2016 at 07:19:10AM +0100, Takashi Iwai wrote: > > > On Fri, 22 Jan 2016 06:46:52 +0100, > > > Vinod Koul wrote: > > > > > > > > TLV byte control are new type of byte controls added in kernel where > > > > controls can have large sizes. > > > > > > > > For these controls querying with 4096 size fails, so use the queried size to > > > > read the control > > > > > > > > This fixes the crash with current cget/contents on these type of controls > > > > > > Hmm... Theoretically the TLV size and the element size are > > > independent. So, it's not good to check only the type being > > > SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, > > > too. > > > > > > Basically it is an abuse in ASoC side to return the size of > > > TLV by the element's info. Usually such value would lead to crash, > > > but the unique point of ASoC ext bytes ctl is that it doesn't have RW > > > accesses but only TLV_RW accesses. > > > > But isn't that already checked? Since we are in the code which will be > > executed only for tlv as snd_ctl_elem_info_is_tlv_readable() ensures that. > > So this is only for tlv + bytes ... > > I meant setting a bogus count value in info would usually lead to a > crash. The point isn't about TLV access. > > > > So, instead of only checking the type being BYTES, check the > > > accesses. Only when all these conditions are met, we may take the > > > count as TLV (element) size. (And still we should have the sanity > > > check of the value, too.) > > > > > > Yet, this isn't a really "fix" for the crash. Even without the patch > > > it shouldn't crash -- it should receive 4096 bytes, and tries to > > > decode. Where did you get the crash exactly? > > > > in alsa-lib snd_ctl_hw_elem_tlv() when it tries to do memcpy for tlv read. > > But the crash is caused as tlv read is large and we have only 4096 size > > buffer, > > Hm, it has a check > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > unsigned int numid, > unsigned int *tlv, unsigned int tlv_size) > { > .... > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > free(xtlv); > return -EFAULT; > } > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > Do you mean somewhere here triggers a crash? > > > so we ensure we give right size buffer here > > Actually we should have an API function that returns the size of TLV. > Then amixer can adjust the allocation size. .... thinking of this again, it might be easier to fix amixer locally at first. Takashi
On Fri, Jan 22, 2016 at 09:09:13AM +0100, Takashi Iwai wrote: > On Fri, 22 Jan 2016 08:46:23 +0100, > Vinod Koul wrote: > > > > On Fri, Jan 22, 2016 at 07:19:10AM +0100, Takashi Iwai wrote: > > > On Fri, 22 Jan 2016 06:46:52 +0100, > > > Vinod Koul wrote: > > > > > > > > TLV byte control are new type of byte controls added in kernel where > > > > controls can have large sizes. > > > > > > > > For these controls querying with 4096 size fails, so use the queried size to > > > > read the control > > > > > > > > This fixes the crash with current cget/contents on these type of controls > > > > > > Hmm... Theoretically the TLV size and the element size are > > > independent. So, it's not good to check only the type being > > > SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, > > > too. > > > > > > Basically it is an abuse in ASoC side to return the size of > > > TLV by the element's info. Usually such value would lead to crash, > > > but the unique point of ASoC ext bytes ctl is that it doesn't have RW > > > accesses but only TLV_RW accesses. > > > > But isn't that already checked? Since we are in the code which will be > > executed only for tlv as snd_ctl_elem_info_is_tlv_readable() ensures that. > > So this is only for tlv + bytes ... > > I meant setting a bogus count value in info would usually lead to a > crash. The point isn't about TLV access. > > > > So, instead of only checking the type being BYTES, check the > > > accesses. Only when all these conditions are met, we may take the > > > count as TLV (element) size. (And still we should have the sanity > > > check of the value, too.) > > > > > > Yet, this isn't a really "fix" for the crash. Even without the patch > > > it shouldn't crash -- it should receive 4096 bytes, and tries to > > > decode. Where did you get the crash exactly? > > > > in alsa-lib snd_ctl_hw_elem_tlv() when it tries to do memcpy for tlv read. > > But the crash is caused as tlv read is large and we have only 4096 size > > buffer, > > Hm, it has a check > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > unsigned int numid, > unsigned int *tlv, unsigned int tlv_size) > { > .... > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > free(xtlv); > return -EFAULT; > } > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > Do you mean somewhere here triggers a crash? Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I allocated right one and got all the data > > > so we ensure we give right size buffer here > > Actually we should have an API function that returns the size of TLV. > Then amixer can adjust the allocation size. Yes we can add tat too. I want to make coontents and cget work okay while I add proper TLV support. Maybe we should stop cget on tlv byte type?
On Fri, Jan 22, 2016 at 09:26:37AM +0100, Takashi Iwai wrote: > > > so we ensure we give right size buffer here > > > > Actually we should have an API function that returns the size of TLV. > > Then amixer can adjust the allocation size. > > .... thinking of this again, it might be easier to fix amixer locally > at first. Sure, so what is approach that you would like me to take
On Fri, 22 Jan 2016 10:56:48 +0100, Vinod Koul wrote: > > On Fri, Jan 22, 2016 at 09:09:13AM +0100, Takashi Iwai wrote: > > On Fri, 22 Jan 2016 08:46:23 +0100, > > Vinod Koul wrote: > > > > > > On Fri, Jan 22, 2016 at 07:19:10AM +0100, Takashi Iwai wrote: > > > > On Fri, 22 Jan 2016 06:46:52 +0100, > > > > Vinod Koul wrote: > > > > > > > > > > TLV byte control are new type of byte controls added in kernel where > > > > > controls can have large sizes. > > > > > > > > > > For these controls querying with 4096 size fails, so use the queried size to > > > > > read the control > > > > > > > > > > This fixes the crash with current cget/contents on these type of controls > > > > > > > > Hmm... Theoretically the TLV size and the element size are > > > > independent. So, it's not good to check only the type being > > > > SND_CTL_ELEM_TYPE_BYTES. This can be used legally by other cases, > > > > too. > > > > > > > > Basically it is an abuse in ASoC side to return the size of > > > > TLV by the element's info. Usually such value would lead to crash, > > > > but the unique point of ASoC ext bytes ctl is that it doesn't have RW > > > > accesses but only TLV_RW accesses. > > > > > > But isn't that already checked? Since we are in the code which will be > > > executed only for tlv as snd_ctl_elem_info_is_tlv_readable() ensures that. > > > So this is only for tlv + bytes ... > > > > I meant setting a bogus count value in info would usually lead to a > > crash. The point isn't about TLV access. > > > > > > So, instead of only checking the type being BYTES, check the > > > > accesses. Only when all these conditions are met, we may take the > > > > count as TLV (element) size. (And still we should have the sanity > > > > check of the value, too.) > > > > > > > > Yet, this isn't a really "fix" for the crash. Even without the patch > > > > it shouldn't crash -- it should receive 4096 bytes, and tries to > > > > decode. Where did you get the crash exactly? > > > > > > in alsa-lib snd_ctl_hw_elem_tlv() when it tries to do memcpy for tlv read. > > > But the crash is caused as tlv read is large and we have only 4096 size > > > buffer, > > > > Hm, it has a check > > > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > > unsigned int numid, > > unsigned int *tlv, unsigned int tlv_size) > > { > > .... > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > free(xtlv); > > return -EFAULT; > > } > > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > > > Do you mean somewhere here triggers a crash? > > Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I > allocated right one and got all the data But it has the size check before memcpy()? Takashi
On Fri, Jan 22, 2016 at 11:50:08AM +0100, Takashi Iwai wrote: > On Fri, 22 Jan 2016 10:56:48 +0100, > Vinod Koul wrote: > > > Hm, it has a check > > > > > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > > > unsigned int numid, > > > unsigned int *tlv, unsigned int tlv_size) > > > { > > > .... > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > > free(xtlv); > > > return -EFAULT; > > > } > > > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > > > > > Do you mean somewhere here triggers a crash? > > > > Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I > > allocated right one and got all the data > > But it has the size check before memcpy()? Okay I had some issues (yet again) so I rebuilt the ubuntu packages and tools and reran this. I get: $ amixer -c0 cget numid=16 numid=16,iface=MIXER,name='mdl params' ; type=BYTES,access=-----RW-,values=30336 amixer: Control hw:0 element TLV read error: Bad address Segmentation fault (core dumped) And the alsa-lib returns an err and we try to free up tlv on error which should be fine here but fails Why should free cause a failure here (on 1.0.27) On latest lib from git. I get same failure but in alsa-lib when it tries to free xtlv on failure of this check if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { free(xtlv); Why would that happen...? What am I missing :( Also on second thought, even after working around this by setting right size so that we don't hit failures (this patch), I get my screen spammed by 30K byes so based on TLV access and bytes type I am thinking to not read and display on cget. We should add new options for these controls ..
On Wed, 27 Jan 2016 18:47:52 +0100, Vinod Koul wrote: > > On Fri, Jan 22, 2016 at 11:50:08AM +0100, Takashi Iwai wrote: > > On Fri, 22 Jan 2016 10:56:48 +0100, > > Vinod Koul wrote: > > > > Hm, it has a check > > > > > > > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > > > > unsigned int numid, > > > > unsigned int *tlv, unsigned int tlv_size) > > > > { > > > > .... > > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > > > free(xtlv); > > > > return -EFAULT; > > > > } > > > > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > > > > > > > Do you mean somewhere here triggers a crash? > > > > > > Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I > > > allocated right one and got all the data > > > > But it has the size check before memcpy()? > > Okay I had some issues (yet again) so I rebuilt the ubuntu packages and tools and reran > this. > > I get: > > $ amixer -c0 cget numid=16 > numid=16,iface=MIXER,name='mdl params' > ; type=BYTES,access=-----RW-,values=30336 > amixer: Control hw:0 element TLV read error: Bad address > > Segmentation fault (core dumped) > > And the alsa-lib returns an err and we try to free up tlv on error which > should be fine here but fails > > Why should free cause a failure here (on 1.0.27) > > On latest lib from git. I get same failure but in alsa-lib when it tries to > free xtlv on failure of this check > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > free(xtlv); > > Why would that happen...? What am I missing :( No idea. You should check it via gdb. > Also on second thought, even after working around this by setting right size > so that we don't hit failures (this patch), I get my screen spammed by 30K byes so based > on TLV access and bytes type I am thinking to not read and display on cget. > > We should add new options for these controls .. As already mentioned, we may handle differently for ctl elements that have no r/w access. At easiest, just skip tlv read for such an element. Takashi
On Wed, Jan 27, 2016 at 07:49:00PM +0100, Takashi Iwai wrote: > On Wed, 27 Jan 2016 18:47:52 +0100, > Vinod Koul wrote: > > > > On Fri, Jan 22, 2016 at 11:50:08AM +0100, Takashi Iwai wrote: > > > On Fri, 22 Jan 2016 10:56:48 +0100, > > > Vinod Koul wrote: > > > > > Hm, it has a check > > > > > > > > > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > > > > > unsigned int numid, > > > > > unsigned int *tlv, unsigned int tlv_size) > > > > > { > > > > > .... > > > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > > > > free(xtlv); > > > > > return -EFAULT; > > > > > } > > > > > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > > > > > > > > > Do you mean somewhere here triggers a crash? > > > > > > > > Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I > > > > allocated right one and got all the data > > > > > > But it has the size check before memcpy()? > > > > Okay I had some issues (yet again) so I rebuilt the ubuntu packages and tools and reran > > this. > > > > I get: > > > > $ amixer -c0 cget numid=16 > > numid=16,iface=MIXER,name='mdl params' > > ; type=BYTES,access=-----RW-,values=30336 > > amixer: Control hw:0 element TLV read error: Bad address > > > > Segmentation fault (core dumped) > > > > And the alsa-lib returns an err and we try to free up tlv on error which > > should be fine here but fails > > > > Why should free cause a failure here (on 1.0.27) > > > > On latest lib from git. I get same failure but in alsa-lib when it tries to > > free xtlv on failure of this check > > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > free(xtlv); > > > > Why would that happen...? What am I missing :( > > No idea. You should check it via gdb. Yes did, but didn't get a clue. tlv seems to be valid before free and points to right location and contents > > Also on second thought, even after working around this by setting right size > > so that we don't hit failures (this patch), I get my screen spammed by 30K byes so based > > on TLV access and bytes type I am thinking to not read and display on cget. > > > > We should add new options for these controls .. > > As already mentioned, we may handle differently for ctl elements that > have no r/w access. At easiest, just skip tlv read for such an > element. Yes will send a patch for this :)
On Thu, 28 Jan 2016 05:25:09 +0100, Vinod Koul wrote: > > On Wed, Jan 27, 2016 at 07:49:00PM +0100, Takashi Iwai wrote: > > On Wed, 27 Jan 2016 18:47:52 +0100, > > Vinod Koul wrote: > > > > > > On Fri, Jan 22, 2016 at 11:50:08AM +0100, Takashi Iwai wrote: > > > > On Fri, 22 Jan 2016 10:56:48 +0100, > > > > Vinod Koul wrote: > > > > > > Hm, it has a check > > > > > > > > > > > > static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, > > > > > > unsigned int numid, > > > > > > unsigned int *tlv, unsigned int tlv_size) > > > > > > { > > > > > > .... > > > > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > > > > > free(xtlv); > > > > > > return -EFAULT; > > > > > > } > > > > > > memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); > > > > > > > > > > > > Do you mean somewhere here triggers a crash? > > > > > > > > > > Yes while it tried to memcpy, in my case 30K sized data to 4K buffer, so I > > > > > allocated right one and got all the data > > > > > > > > But it has the size check before memcpy()? > > > > > > Okay I had some issues (yet again) so I rebuilt the ubuntu packages and tools and reran > > > this. > > > > > > I get: > > > > > > $ amixer -c0 cget numid=16 > > > numid=16,iface=MIXER,name='mdl params' > > > ; type=BYTES,access=-----RW-,values=30336 > > > amixer: Control hw:0 element TLV read error: Bad address > > > > > > Segmentation fault (core dumped) > > > > > > And the alsa-lib returns an err and we try to free up tlv on error which > > > should be fine here but fails > > > > > > Why should free cause a failure here (on 1.0.27) > > > > > > On latest lib from git. I get same failure but in alsa-lib when it tries to > > > free xtlv on failure of this check > > > > > > if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { > > > free(xtlv); > > > > > > Why would that happen...? What am I missing :( > > > > No idea. You should check it via gdb. > > Yes did, but didn't get a clue. tlv seems to be valid before free and points > to right location and contents Could you show the backtrace? Takashi
On Thu, Jan 28, 2016 at 06:49:48AM +0100, Takashi Iwai wrote: > > Yes did, but didn't get a clue. tlv seems to be valid before free and points > > to right location and contents > > Could you show the backtrace? 676 if ((err = snd_hctl_elem_tlv_read(elem, tlv, 4096)) < 0) { (gdb) n 677 error("Control %s element TLV read error: %s\n", card, snd_strerror(err)); (gdb) p err $1 = -14 (gdb) n amixer: Control hw:0 element TLV read error: Bad address 678 free(tlv); (gdb) p tlv $2 = (unsigned int *) 0x625f10 (gdb) p tlv[0] $3 = 4294967295 (gdb) p tlv[1] $4 = 0 (gdb) backtrace #0 show_control (elem=0x625310, level=level@entry=5, space=0x409b01 " ") at amixer.c:678 #1 0x00000000004066d9 in cset (argc=argc@entry=1, argv=0x7fffffffe530, roflag=roflag@entry=1, keep_handle=keep_handle@entry=0) at amixer.c:1184 #2 0x0000000000404352 in main (argc=<optimized out>, argv=0x7fffffffe518) at amixer.c:1863 (gdb) c Continuing. Program received signal SIGSEGV, Segmentation fault. 0x00007ffff749e12f in _int_free (av=0x7ffff77dd760 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:3996 3996 malloc.c: No such file or directory. (gdb) backtrace #0 0x00007ffff749e12f in _int_free (av=0x7ffff77dd760 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:3996 #1 0x00000000004063be in show_control (elem=0x625310, level=level@entry=5, space=0x409b01 " ") at amixer.c:678 #2 0x00000000004066d9 in cset (argc=argc@entry=1, argv=0x7fffffffe530, roflag=roflag@entry=1, keep_handle=keep_handle@entry=0) at amixer.c:1184 #3 0x0000000000404352 in main (argc=<optimized out>, argv=0x7fffffffe518) at amixer.c:1863
On Thu, 28 Jan 2016 10:26:44 +0100, Vinod Koul wrote: > > On Thu, Jan 28, 2016 at 06:49:48AM +0100, Takashi Iwai wrote: > > > Yes did, but didn't get a clue. tlv seems to be valid before free and points > > > to right location and contents > > > > Could you show the backtrace? > > 676 if ((err = snd_hctl_elem_tlv_read(elem, tlv, 4096)) < 0) { > (gdb) n > 677 error("Control %s element TLV read error: %s\n", card, snd_strerror(err)); > (gdb) p err > $1 = -14 > > (gdb) n > amixer: Control hw:0 element TLV read error: Bad address > > 678 free(tlv); > (gdb) p tlv > $2 = (unsigned int *) 0x625f10 > (gdb) p tlv[0] > $3 = 4294967295 > (gdb) p tlv[1] > $4 = 0 > (gdb) backtrace > #0 show_control (elem=0x625310, level=level@entry=5, space=0x409b01 " ") > at amixer.c:678 > #1 0x00000000004066d9 in cset (argc=argc@entry=1, argv=0x7fffffffe530, > roflag=roflag@entry=1, keep_handle=keep_handle@entry=0) at amixer.c:1184 > #2 0x0000000000404352 in main (argc=<optimized out>, argv=0x7fffffffe518) > at amixer.c:1863 > (gdb) c > Continuing. The line number doesn't match with the latest code in git, so double-check that the problem happens with the latest alsa-lib and alsa-utils, too. I'm thinking whether this is rather an issue in the kernel driver side. In skl_tplg_tlv_control_get(), if (bc->params) { if (copy_to_user(data, &bc->param_id, sizeof(u32))) return -EFAULT; if (copy_to_user(data + 1, &size, sizeof(u32))) return -EFAULT; if (copy_to_user(data + 2, bc->params, size)) return -EFAULT; } But here, size is the size of the whole container, not the size in the container. In the code above, you're copying size+8 bytes total and this breaks the boundary already. Takashi
On Thu, Jan 28, 2016 at 05:19:21PM +0100, Takashi Iwai wrote: > The line number doesn't match with the latest code in git, so > double-check that the problem happens with the latest alsa-lib and > alsa-utils, too. I am on debian packages 1.0.27 > I'm thinking whether this is rather an issue in the kernel driver > side. In skl_tplg_tlv_control_get(), I think you are right, the buffer would overflow which would cause heap to go bad and free goes crashing > > if (bc->params) { > if (copy_to_user(data, &bc->param_id, sizeof(u32))) > return -EFAULT; > if (copy_to_user(data + 1, &size, sizeof(u32))) > return -EFAULT; > if (copy_to_user(data + 2, bc->params, size)) > return -EFAULT; > } > > But here, size is the size of the whole container, not the size in the > container. In the code above, you're copying size+8 bytes total and > this breaks the boundary already. Right, also I think we need to check for size vs size of parameters. We don't want to copy kernel memory to usermode if usermode gave a larger buffer Let me test this, thanks for pointing
On Fri, Jan 29, 2016 at 12:21:06PM +0530, Vinod Koul wrote: > On Thu, Jan 28, 2016 at 05:19:21PM +0100, Takashi Iwai wrote: > > The line number doesn't match with the latest code in git, so > > double-check that the problem happens with the latest alsa-lib and > > alsa-utils, too. > > I am on debian packages 1.0.27 > > > I'm thinking whether this is rather an issue in the kernel driver > > side. In skl_tplg_tlv_control_get(), > > I think you are right, the buffer would overflow which would cause heap to > go bad and free goes crashing > > > > > if (bc->params) { > > if (copy_to_user(data, &bc->param_id, sizeof(u32))) > > return -EFAULT; > > if (copy_to_user(data + 1, &size, sizeof(u32))) > > return -EFAULT; > > if (copy_to_user(data + 2, bc->params, size)) > > return -EFAULT; > > } > > > > But here, size is the size of the whole container, not the size in the > > container. In the code above, you're copying size+8 bytes total and > > this breaks the boundary already. > > Right, also I think we need to check for size vs size of parameters. We > don't want to copy kernel memory to usermode if usermode gave a larger > buffer > > Let me test this, thanks for pointing And you were right :) with this change it works and dumps 4K bytes on my screen @@ -913,6 +913,13 @@ static int skl_tplg_tlv_control_get(struct snd_kcontrol *kcontrol, skl_get_module_params(skl->skl_sst, (u32 *)bc->params, bc->max, bc->param_id, mconfig); + /* decrement size for TLV header */ + size -= 2 * sizeof(u32); + + /* check size as we don't want to send kernel data */ + if (size > bc->max) + size = bc->max; + if (bc->params) { if (copy_to_user(data, &bc->param_id, sizeof(u32))) return -EFAULT; Thanks
On Fri, 29 Jan 2016 12:13:47 +0100, Vinod Koul wrote: > > On Fri, Jan 29, 2016 at 12:21:06PM +0530, Vinod Koul wrote: > > On Thu, Jan 28, 2016 at 05:19:21PM +0100, Takashi Iwai wrote: > > > The line number doesn't match with the latest code in git, so > > > double-check that the problem happens with the latest alsa-lib and > > > alsa-utils, too. > > > > I am on debian packages 1.0.27 > > > > > I'm thinking whether this is rather an issue in the kernel driver > > > side. In skl_tplg_tlv_control_get(), > > > > I think you are right, the buffer would overflow which would cause heap to > > go bad and free goes crashing > > > > > > > > if (bc->params) { > > > if (copy_to_user(data, &bc->param_id, sizeof(u32))) > > > return -EFAULT; > > > if (copy_to_user(data + 1, &size, sizeof(u32))) > > > return -EFAULT; > > > if (copy_to_user(data + 2, bc->params, size)) > > > return -EFAULT; > > > } > > > > > > But here, size is the size of the whole container, not the size in the > > > container. In the code above, you're copying size+8 bytes total and > > > this breaks the boundary already. > > > > Right, also I think we need to check for size vs size of parameters. We > > don't want to copy kernel memory to usermode if usermode gave a larger > > buffer > > > > Let me test this, thanks for pointing > > And you were right :) Good to hear :) > with this change it works and dumps 4K bytes on my screen > > @@ -913,6 +913,13 @@ static int skl_tplg_tlv_control_get(struct snd_kcontrol > *kcontrol, > skl_get_module_params(skl->skl_sst, (u32 *)bc->params, > bc->max, bc->param_id, mconfig); > > + /* decrement size for TLV header */ > + size -= 2 * sizeof(u32); The lower bound check is also missing. if (size < 2 * sizeof(u32)) return -EINVAL; Takashi > + > + /* check size as we don't want to send kernel data */ > + if (size > bc->max) > + size = bc->max; > + > if (bc->params) { > if (copy_to_user(data, &bc->param_id, sizeof(u32))) > return -EFAULT; > > Thanks > -- > ~Vinod >
On Fri, Jan 29, 2016 at 02:17:20PM +0100, Takashi Iwai wrote: > > with this change it works and dumps 4K bytes on my screen > > > > @@ -913,6 +913,13 @@ static int skl_tplg_tlv_control_get(struct snd_kcontrol > > *kcontrol, > > skl_get_module_params(skl->skl_sst, (u32 *)bc->params, > > bc->max, bc->param_id, mconfig); > > > > + /* decrement size for TLV header */ > > + size -= 2 * sizeof(u32); > > The lower bound check is also missing. > > if (size < 2 * sizeof(u32)) > return -EINVAL; I thought core and lib do check this, but yes no harm in adding will do so Thanks
On Fri, 29 Jan 2016 14:53:44 +0100, Vinod Koul wrote: > > On Fri, Jan 29, 2016 at 02:17:20PM +0100, Takashi Iwai wrote: > > > with this change it works and dumps 4K bytes on my screen > > > > > > @@ -913,6 +913,13 @@ static int skl_tplg_tlv_control_get(struct snd_kcontrol > > > *kcontrol, > > > skl_get_module_params(skl->skl_sst, (u32 *)bc->params, > > > bc->max, bc->param_id, mconfig); > > > > > > + /* decrement size for TLV header */ > > > + size -= 2 * sizeof(u32); > > > > The lower bound check is also missing. > > > > if (size < 2 * sizeof(u32)) > > return -EINVAL; > > I thought core and lib do check this, but yes no harm in adding will do so Ah OK, snd_ctl_tlv_ioctl() has the check already. I checked only snd_soc_bytes_tlv_callback(), so far. thanks, Takashi
diff --git a/amixer/amixer.c b/amixer/amixer.c index db1849333da3..cfe13592347f 100644 --- a/amixer/amixer.c +++ b/amixer/amixer.c @@ -588,7 +588,7 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, int level) { int err; - unsigned int item, idx, count, *tlv; + unsigned int item, idx, count, *tlv, tlv_sz; snd_ctl_elem_type_t type; snd_ctl_elem_id_t *id; snd_ctl_elem_info_t *info; @@ -682,13 +682,20 @@ static int show_control(const char *space, snd_hctl_elem_t *elem, __skip_read: if (!snd_ctl_elem_info_is_tlv_readable(info)) goto __skip_tlv; - tlv = malloc(4096); - if ((err = snd_hctl_elem_tlv_read(elem, tlv, 4096)) < 0) { + + if (type == SND_CTL_ELEM_TYPE_BYTES) + tlv_sz = count + 2 * sizeof(unsigned int); + else + tlv_sz = 4096; + + tlv = malloc(tlv_sz); + + if ((err = snd_hctl_elem_tlv_read(elem, tlv, tlv_sz)) < 0) { error("Control %s element TLV read error: %s\n", card, snd_strerror(err)); free(tlv); return err; } - decode_tlv(strlen(space), tlv, 4096); + decode_tlv(strlen(space), tlv, tlv_sz); free(tlv); } __skip_tlv:
TLV byte control are new type of byte controls added in kernel where controls can have large sizes. For these controls querying with 4096 size fails, so use the queried size to read the control This fixes the crash with current cget/contents on these type of controls Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- amixer/amixer.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)