From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] amixer: add support for TLV byte control read Date: Fri, 22 Jan 2016 09:26:37 +0100 Message-ID: References: <1453441612-29902-1-git-send-email-vinod.koul@intel.com> <20160122074623.GC11130@localhost> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 715FC260439 for ; Fri, 22 Jan 2016 09:26:38 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Vinod Koul Cc: liam.r.girdwood@linux.intel.com, patches.audio@intel.com, alsa-devel@alsa-project.org, broonie@kernel.org List-Id: alsa-devel@alsa-project.org 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