Re: [PATCH v3 3/3] ASoC: Intel: Add cht_rt5677 driver

[Yauhen Kharuzhy <jekhor@gmail.com>]

On Thu, Jun 11, 2026 at 11:19:10AM +0200, Cezary Rojewski wrote:
> On 6/11/2026 1:51 AM, Yauhen Kharuzhy wrote:
> > Add a new ASoC machine driver for Intel Cherry Trail platforms with
> > rt5677 codec.
> 
> ...
> > +static void snd_cht_rt5677_remove(struct platform_device *pdev)
> > +{
> > +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> > +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(card);
> > +	int i;
> > +
> > +	/*
> > +	 * Reset the codec name in the global dailink array back to the default
> > +	 * to avoid a use-after-free on driver rebind (unbind/bind):
> > +	 * ctx->codec_name is about to be freed together with ctx (devm), but
> > +	 * cht_rt5677_dailink[] is a static global that persists across binds.
> > +	 */
> 
> All of this looks like a hack and brings me closer to simply stating:
> 
> NAK
> 
> as I do not believe the patch has been thoroughly tested if such hacks
> exist.  remove() procedure should not care about codecs->name.

I agree but it was a simplest way to fix a possible use-after-free in
probe():

ctx = kzalloc();
...
if (cht_rt5677_dailink[i].codecs->name &&
		!strcmp(cht_rt5677_dailink[i].codecs->name,
			RT5677_I2C_DEFAULT))
	cht_rt5677_dailink[i].codecs->name = ctx->codec_name;

So, if ctx will be freed (after remove()) and probe() will be called
again, then cht_rt5677_dailink[i].codecs->name will be invalid.

Now I have checked how this issue is resolved in similar drivers. For
example, cht_bsw_rt5672.c has the same pattern with potential
use-after-free. cht_bsw_rt5645.c uses a statically allocated buffer to
store the name instead of field inside a dynamically allocated context.
Maybe a such approach with static buffer would be an acceptable compromise.


> 
> > +	for (i = 0; i < ARRAY_SIZE(cht_rt5677_dailink); i++) {
> > +		if (cht_rt5677_dailink[i].codecs->name == ctx->codec_name) {
> > +			cht_rt5677_dailink[i].codecs->name = RT5677_I2C_DEFAULT;
> > +			break;
> > +		}
> > +	}
> > +
> > +	gpiod_put(ctx->gpio_hp_en);
> > +	gpiod_put(ctx->gpio_spk_en2);
> > +	gpiod_put(ctx->gpio_spk_en1);
> > +}
> 
> ...
> 
> > diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> > index 5e8a1dc84ee1..c719c3ec8314 100644
> > --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> > +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> > @@ -17,12 +17,11 @@ static struct snd_soc_acpi_mach cht_surface_mach = {
> >   	.sof_tplg_filename = "sof-cht-rt5645.tplg",
> >   };
> > -static struct snd_soc_acpi_mach cht_yogabook_mach = {
> > +static struct snd_soc_acpi_mach cht_rt5677_mach = {
> >   	.id = "10EC5677",
> > -	.drv_name = "cht-yogabook",
> > +	.drv_name = "cht-rt5677",
> >   	.fw_filename = "intel/fw_sst_22a8.bin",
> > -	.board = "cht-yogabook",
> > -	.sof_tplg_filename = "sof-cht-rt5677.tplg",
> > +	.board = "cht_rt5677",
> >   };
> >   static struct snd_soc_acpi_mach cht_lenovo_yoga_tab3_x90_mach = {
> > @@ -41,17 +40,9 @@ static const struct dmi_system_id cht_table[] = {
> >   			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> >   		},
> >   	},
> > -	{
> > -		.ident = "Lenovo Yoga Book YB1-X91",
> > -		.driver_data = (void *)&cht_yogabook_mach,
> > -		/* YB1-X91L/F */
> > -		.matches = {
> > -			DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"),
> > -		}
> > -	},
> >   	{
> >   		.ident = "Lenovo Yoga Book YB1-X90",
> > -		.driver_data = (void *)&cht_yogabook_mach,
> > +		.driver_data = (void *)&cht_rt5677_mach,
> >   		/* YB1-X90L/F, codec is not listed in DSDT */
> >   		.matches = {
> >   			
> > @@ -147,19 +138,11 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
> >   		.board = "cht-bsw",
> >   		.sof_tplg_filename = "sof-cht-rt5670.tplg",
> >   	},
> > -	/*
> > -	 * The only known Cherry Trail device with RT5677 codec and 10EC677
> > -	 * DSTD entry is the Lenovo Yoga Book YB1-X91. It has a device-specific
> > -	 * driver, so check DMI and use a machine quirk to override the default
> > -	 * (non-existent) machine driver.
> > -	 */
> 
> You are removing and editing (the below table) code you've just added with
> 2/3.  This looks very bad.

Argh, it should have been integrated into the second patch in the series
instead of this one... My bad, sorry.

> 
> >   	{
> >   		.id = "10EC5677",
> > -		.drv_name = "cht-bsw-rt5677",
> > +		.drv_name = "cht-rt5677",
> >   		.fw_filename = "intel/fw_sst_22a8.bin",
> > -		.board = "cht-bsw",
> > -		.machine_quirk = cht_quirk,
> > -		.sof_tplg_filename = "sof-cht-rt5677.tplg",
> > +		.board = "cht_rt5677",
> >   	},
> >   	{
> >   		.comp_ids = &rt5645_comp_ids,
> > 
> 

-- 
Yauhen Kharuzhy