[PATCH] amixer: add support for TLV byte control read

[PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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(-)

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

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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
> 

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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

-- 
~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
> > 

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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
> > > 
> 

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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?

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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 ..

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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 :)

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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
-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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
> 

Re: [PATCH] amixer: add support for TLV byte control read

[Vinod Koul]

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
-- 
~Vinod

Re: [PATCH] amixer: add support for TLV byte control read

[Takashi Iwai]

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