| Message ID | 1418282517-851-1-git-send-email-kevin.strasser@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)
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(-)