Re: Question on compress offload framework memory corruption

["Banajit Goswami" <bgoswami@codeaurora.org>]

Hi Liam,

> On Thu, Dec 19, 2013 at 10:55:08AM +0000, Mark Brown wrote:
> > On Wed, Dec 18, 2013 at 01:17:51PM -0000, gsantosh@codeaurora.org wrote:
> > > Hi,
> > >
> > > I have following questions in the compressed offload framework API.
> > >
> > > static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
> > > 					struct snd_compr_params *params) { ...
> > > 	hw_params = kzalloc(sizeof(*hw_params), GFP_KERNEL);
> > > 	if (hw_params == NULL)
> > > 		return -ENOMEM;
> > > /*1st question is, what is the use of above allocated memory I do
> > > not see this being used in this function*/
> >
> > The code you're quoting there isn't in mainline.  I've also added
> > Vinod to the CCs, you probably want to include him in any discussion
> > of the compressed API.
>
> That's right, this is from DPCM compressed updated done by Liam, so he
is > the best person to comment on this bit, and somehow his address in
CC has > gone bad, i have added him in to list

> But yes looking at the source, i agree that two are interlinked.
> I guess we need to poupulate the hw_params and then use that to copy, that
> would be the right thing to do here and yes this is not correct and not
> sure how this is working...


Continuing the discussion on the "memcpy" function in Santosh's 2nd question-

> > >        memcpy(&fe->dpcm[fe_substream->stream].hw_params, params,
> > >                       sizeof(struct snd_pcm_hw_params));
> > >
> > > /* 2nd question is
> > > in above memcpy there is parameter mismatch
> > > &fe->dpcm[fe_substream->stream].hw_params is of structure type struct
> > > snd_pcm_hw_params
> > >
> > > params argument is of the structure type struct snd_compr_params
> > > the definition of the two structures are different, how this is
> > > working?
> > > this will over right the destination with junk value which will return
> > > in error when this memory is accessed, after memcpy this cpu_dai
> > > hw_params
> > > returing with EINVAL error, please explain why this is done this way.
> > > */

I can see that a later version of the patch adds the function
soc_compr_set_params_fe() at-
    http://permalink.gmane.org/gmane.linux.alsa.devel/117716

with the below code replaces the "memcpy" function with a "memset"-
+	/*
+	 * Create an empty hw_params for the BE as the machine driver must
+	 * fix this up to match DSP decoder and ASRC configuration.
+	 * I.e. machine driver fixup for compressed BE is mandatory.
+	 */
+	memset(&fe->dpcm[fe_substream->stream].hw_params, 0,
+		sizeof(struct snd_pcm_hw_params));

It's evident that the code relies on machine driver fixup function to
populate the parameters of hw_params before propagating those to back-end.
I think rather than that, like Vinod also mentioned, the hw_params should
have been populated with individual parameters which are relevant.

Let me know your comment on this.

Thanks,
Banajit

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation