[alsa-devel] [PATCH] ASoC: Intel: fix possible acpi enumeration panic
A crash can occur on some platforms where adsp is enumerated but codec is not matched. Check that the codec_id string is valid before attempting to match.
Signed-off-by: Kevin Strasser kevin.strasser@intel.com --- sound/soc/intel/sst/sst_acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/sst/sst_acpi.c b/sound/soc/intel/sst/sst_acpi.c index 31124aa..dd72e58 100644 --- a/sound/soc/intel/sst/sst_acpi.c +++ b/sound/soc/intel/sst/sst_acpi.c @@ -236,7 +236,7 @@ static struct sst_machines *sst_acpi_find_machine( struct sst_machines *mach; bool found = false;
- for (mach = machines; mach->codec_id; mach++) + for (mach = machines; mach->codec_id[0]; mach++) if (ACPI_SUCCESS(acpi_get_devices(mach->codec_id, sst_acpi_mach_match, &found, NULL)) && found)
On Wed, Dec 10, 2014 at 11:21:57PM -0800, Kevin Strasser wrote:
A crash can occur on some platforms where adsp is enumerated but codec is not matched. Check that the codec_id string is valid before attempting to match.
- for (mach = machines; mach->codec_id; mach++)
- for (mach = machines; mach->codec_id[0]; mach++)
This changes the check from verifying if a codec_id is present to verifying if the first character in the codec_id is non-NULL. That doesn't seem obviously safer and the tables of machines seem to be terminated by having an entry with all fields set to zero (which is a common idiom in Linux) which would now crash with this change.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Thursday, December 11, 2014 5:20 AM> On Wed, Dec 10, 2014 at 11:21:57PM -0800, Kevin Strasser wrote:
A crash can occur on some platforms where adsp is enumerated but codec is not matched. Check that the codec_id string is valid before attempting to match.
- for (mach = machines; mach->codec_id; mach++)
- for (mach = machines; mach->codec_id[0]; mach++)
This changes the check from verifying if a codec_id is present to verifying if the first character in the codec_id is non-NULL. That doesn't seem obviously safer and the tables of machines seem to be terminated by having an entry with all fields set to zero (which is a common idiom in Linux) which would now crash with this change.
In this case mach->codec_id is non-NULL, even for the terminating element, because it is defined to be a fixed width. So we have to take a look at the first character to see if it has been initialized.
-Kevin
On Thu, Dec 11, 2014 at 09:55:38PM +0000, Strasser, Kevin wrote:
Please fix your mailer to word wrap comfortably under 80 colums so that your mails are easily legible.
This changes the check from verifying if a codec_id is present to verifying if the first character in the codec_id is non-NULL. That doesn't seem obviously safer and the tables of machines seem to be terminated by having an entry with all fields set to zero (which is a common idiom in Linux) which would now crash with this change.
In this case mach->codec_id is non-NULL, even for the terminating element, because it is defined to be a fixed width. So we have to take a look at the first character to see if it has been initialized.
That's a really unusual and (as you've seen) error prone idiom - is it not better to fix the struct to use the more common idiom?
On Mon, Dec 15, 2014 at 05:06:45PM +0000, Mark Brown wrote:
Please fix your mailer to word wrap comfortably under 80 colums so that your mails are easily legible.
Understood
This changes the check from verifying if a codec_id is present to verifying if the first character in the codec_id is non-NULL. That doesn't seem obviously safer and the tables of machines seem to be terminated by having an entry with all fields set to zero (which is a common idiom in Linux) which would now crash with this change.
In this case mach->codec_id is non-NULL, even for the terminating element, because it is defined to be a fixed width. So we have to take a look at the first character to see if it has been initialized.
That's a really unusual and (as you've seen) error prone idiom - is it not better to fix the struct to use the more common idiom?
That seems like a good idea to me. I'll prepare a new patch to change the sst_machines definition so that codec_id gets initialized to NULL.
-Kevin
A crash can occur on some platforms where adsp is enumerated but codec is not matched. Define codec_id as a pointer intead of an array so that it gets initialized to NULL for the terminating element of sst_acpi_bytcr[] and sst_acpi_chv[].
Signed-off-by: Kevin Strasser kevin.strasser@linux.intel.com --- sound/soc/intel/sst/sst_acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/sst/sst_acpi.c b/sound/soc/intel/sst/sst_acpi.c index 31124aa..87b5090 100644 --- a/sound/soc/intel/sst/sst_acpi.c +++ b/sound/soc/intel/sst/sst_acpi.c @@ -43,7 +43,7 @@ #include "sst.h"
struct sst_machines { - char codec_id[32]; + char *codec_id; char board[32]; char machine[32]; void (*machine_quirk)(void);
On Mon, Dec 15, 2014 at 04:15:04PM -0800, Kevin Strasser wrote:
A crash can occur on some platforms where adsp is enumerated but codec is not matched. Define codec_id as a pointer intead of an array so that it gets initialized to NULL for the terminating element of sst_acpi_bytcr[] and sst_acpi_chv[].
Applied, thanks.
participants (4)
-
Kevin Strasser -
Kevin Strasser -
Mark Brown -
Strasser, Kevin