Question on compress offload framework memory corruption

Question on compress offload framework memory corruption

[gsantosh]

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

...

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

...

}

Regards,
Santosh M G.

Re: Question on compress offload framework memory corruption

[gsantosh]

Re sending this as i did not got the response.


--Santosh

> 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*/
>
> ...
>
> 	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.
> */
>
> ...
>
> }
>
> Regards,
> Santosh M G.
>
>
>

Re: Question on compress offload framework memory corruption

[Clemens Ladisch]

Re-sending with Liam's correct address (which is _not_ clemens@ladisch.de).

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*/
>
> ...
>
> 	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.
> */
>
> ...
>
> }
>
> Regards,
> Santosh M G.

Re: Question on compress offload framework memory corruption

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 668 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Question on compress offload framework memory corruption

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 1215 bytes --]

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

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Question on compress offload framework memory corruption

[Banajit Goswami]

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

Re: Question on compress offload framework memory corruption

[Vinod Koul]

On Tue, Mar 04, 2014 at 10:19:20PM +0000, Banajit Goswami wrote:
> 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.

Yes I did mention that hw_params should be populated and even gave it a shot.
But this is not so simple. It depends on how decoder is done. The file may tell
you few PCM format details (only for few lucky formats) not all. It wont tell
you PCM word length!  Also you may or may not have SRCs... 

Hence we thought it does not make sense and since we are talking about a DSP we
will end up having some fixed format to which you will process and render data.
So that format would be sensible to be put it fixup.

If you have an idea how to solve this, am all ears :)

-- 
~Vinod