[PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

We cannot use strcpy() to write to a const char * location. This is
causing a 'BUG: unable to handle kernel paging request' error at boot
when using the cht-bsw-rt5645 driver.

With this patch we also fix a wrong indexing in the driver where the
codec_name of the wrong dai_link is being overwritten.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index 2d3afdd..a7b96a9 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -367,8 +367,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	}
 	card->dev = &pdev->dev;
 	sprintf(codec_name, "i2c-%s:00", drv->acpi_card->codec_id);
+
 	/* set correct codec name */
-	strcpy((char *)card->dai_link[2].codec_name, codec_name);
+	for (i = 0; i < ARRAY_SIZE(cht_dailink); i++)
+		if (!strcmp(card->dai_link[i].codec_name, "i2c-10EC5645:00"))
+			card->dai_link[i].codec_name = kstrdup(codec_name, GFP_KERNEL);
+
 	snd_soc_card_set_drvdata(card, drv);
 	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
 	if (ret_val) {
-- 
2.5.0

Re: [PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Mark Brown]

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

On Tue, Feb 23, 2016 at 09:50:20AM +0100, Carlo Caione wrote:
> From: Carlo Caione <carlo@endlessm.com>
> 
> We cannot use strcpy() to write to a const char * location. This is
> causing a 'BUG: unable to handle kernel paging request' error at boot
> when using the cht-bsw-rt5645 driver.
> 
> With this patch we also fix a wrong indexing in the driver where the
> codec_name of the wrong dai_link is being overwritten.

So how was the original code tested then...?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Vinod Koul]

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

On Fri, Feb 26, 2016 at 11:12:10AM +0900, Mark Brown wrote:
> On Tue, Feb 23, 2016 at 09:50:20AM +0100, Carlo Caione wrote:
> > From: Carlo Caione <carlo@endlessm.com>
> > 
> > We cannot use strcpy() to write to a const char * location. This is
> > causing a 'BUG: unable to handle kernel paging request' error at boot
> > when using the cht-bsw-rt5645 driver.
> > 
> > With this patch we also fix a wrong indexing in the driver where the
> > codec_name of the wrong dai_link is being overwritten.
> 
> So how was the original code tested then...?

I know systems using this are shipping so surprised to see this.

Carlo cna you please tell me the configuration where it is seen.

Also would be worth mentioning here that Pierre is working on a super fix
for this which involves removing this code and getting the right IDs from
BIOS, so we wont need this code.

Thanks
-- 
~Vinod

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

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

Re: [PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Carlo Caione]

On Thu, Mar 3, 2016 at 12:32 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Feb 26, 2016 at 11:12:10AM +0900, Mark Brown wrote:
>> On Tue, Feb 23, 2016 at 09:50:20AM +0100, Carlo Caione wrote:
>> > From: Carlo Caione <carlo@endlessm.com>
>> >
>> > We cannot use strcpy() to write to a const char * location. This is
>> > causing a 'BUG: unable to handle kernel paging request' error at boot
>> > when using the cht-bsw-rt5645 driver.
>> >
>> > With this patch we also fix a wrong indexing in the driver where the
>> > codec_name of the wrong dai_link is being overwritten.
>>
>> So how was the original code tested then...?
>
> I know systems using this are shipping so surprised to see this.
>
> Carlo cna you please tell me the configuration where it is seen.

Hi Vinod,
I'm working on the ECS EF20EA cherry-trail laptop. This is the used
configuration:

https://gist.github.com/carlocaione/925540bc7b04eff01aa8

Cheers,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: [PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Pierre-Louis Bossart]

On 3/3/16 5:32 AM, Vinod Koul wrote:
> On Fri, Feb 26, 2016 at 11:12:10AM +0900, Mark Brown wrote:
>> On Tue, Feb 23, 2016 at 09:50:20AM +0100, Carlo Caione wrote:
>>> From: Carlo Caione <carlo@endlessm.com>
>>>
>>> We cannot use strcpy() to write to a const char * location. This is
>>> causing a 'BUG: unable to handle kernel paging request' error at boot
>>> when using the cht-bsw-rt5645 driver.
>>>
>>> With this patch we also fix a wrong indexing in the driver where the
>>> codec_name of the wrong dai_link is being overwritten.
>>
>> So how was the original code tested then...?
>
> I know systems using this are shipping so surprised to see this.
>
> Carlo cna you please tell me the configuration where it is seen.
>
> Also would be worth mentioning here that Pierre is working on a super fix
> for this which involves removing this code and getting the right IDs from
> BIOS, so we wont need this code.

I am actually using the same code to find out which DAI fields need to 
be replaced instead of hard-codec indices so the loop part is a good thing.
There is still a need to copy a string, be it a constant to generated 
from the HID, into the codec_name field. I am not clear why strcpy() 
works on all my systems and why kstrdup() is required. Is the kernel 
issue really due to this string handling or to the bad index?

Re: [PATCH v2] ASoC: cht_bsw_rt5645: Fix writing to string literal

[Carlo Caione]

On Thu, Mar 3, 2016 at 5:11 PM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> On 3/3/16 5:32 AM, Vinod Koul wrote:
>>
>> On Fri, Feb 26, 2016 at 11:12:10AM +0900, Mark Brown wrote:
>>>
>>> On Tue, Feb 23, 2016 at 09:50:20AM +0100, Carlo Caione wrote:
>>>>
>>>> From: Carlo Caione <carlo@endlessm.com>
>>>>
>>>> We cannot use strcpy() to write to a const char * location. This is
>>>> causing a 'BUG: unable to handle kernel paging request' error at boot
>>>> when using the cht-bsw-rt5645 driver.
>>>>
>>>> With this patch we also fix a wrong indexing in the driver where the
>>>> codec_name of the wrong dai_link is being overwritten.
>>>
>>>
>>> So how was the original code tested then...?
>>
>>
>> I know systems using this are shipping so surprised to see this.
>>
>> Carlo cna you please tell me the configuration where it is seen.
>>
>> Also would be worth mentioning here that Pierre is working on a super fix
>> for this which involves removing this code and getting the right IDs from
>> BIOS, so we wont need this code.
>
>
> I am actually using the same code to find out which DAI fields need to be
> replaced instead of hard-codec indices so the loop part is a good thing.
> There is still a need to copy a string, be it a constant to generated from
> the HID, into the codec_name field. I am not clear why strcpy() works on all
> my systems and why kstrdup() is required. Is the kernel issue really due to
> this string handling or to the bad index?

In my tests using kstrdup() fixed the BUG issue and the bad indexing
fix was required to have the driver working correctly.

-- 
Carlo Caione

Applied "ASoC: cht_bsw_rt5645: Fix writing to string literal" to the asoc tree

[Mark Brown]

The patch

   ASoC: cht_bsw_rt5645: Fix writing to string literal

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From c8560b7c917f8738f5d80dd516930edc1d05e4e4 Mon Sep 17 00:00:00 2001
From: Carlo Caione <carlo@endlessm.com>
Date: Tue, 23 Feb 2016 09:50:20 +0100
Subject: [PATCH] ASoC: cht_bsw_rt5645: Fix writing to string literal

We cannot use strcpy() to write to a const char * location. This is
causing a 'BUG: unable to handle kernel paging request' error at boot
when using the cht-bsw-rt5645 driver.

With this patch we also fix a wrong indexing in the driver where the
codec_name of the wrong dai_link is being overwritten.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index 2d3afddb0a2e..a7b96a9a4e0e 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -367,8 +367,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	}
 	card->dev = &pdev->dev;
 	sprintf(codec_name, "i2c-%s:00", drv->acpi_card->codec_id);
+
 	/* set correct codec name */
-	strcpy((char *)card->dai_link[2].codec_name, codec_name);
+	for (i = 0; i < ARRAY_SIZE(cht_dailink); i++)
+		if (!strcmp(card->dai_link[i].codec_name, "i2c-10EC5645:00"))
+			card->dai_link[i].codec_name = kstrdup(codec_name, GFP_KERNEL);
+
 	snd_soc_card_set_drvdata(card, drv);
 	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
 	if (ret_val) {
-- 
2.7.0