Skip to content

module_adapter: fix switch case for spec parsing #9384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kv2019i merged 1 commit into from
Aug 29, 2024
+14 −1
Merged

module_adapter: fix switch case for spec parsing #9384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/audio/module_adapter/module_adapter_ipc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,27 @@ int module_adapter_init_data(struct comp_dev *dev,
data = spec;
break;
}
default:
case SOF_COMP_EQ_IIR:
case SOF_COMP_EQ_FIR:
case SOF_COMP_KEYWORD_DETECT:
case SOF_COMP_KPB:
case SOF_COMP_SELECTOR:
case SOF_COMP_DEMUX:
case SOF_COMP_MUX:
case SOF_COMP_DCBLOCK:
case SOF_COMP_SMART_AMP:
case SOF_COMP_MODULE_ADAPTER:
case SOF_COMP_NONE:

@kv2019i kv2019i Aug 26, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singalsu Can you check this?

The idea was really to have common handling for process types and not need to enumerate process types in so many places. OTOH, this was more of a concern to Linux kernel side (needed to change the Linux kernel for every new SOF module type would not scale). We certainly know the superset of modules that can be used, but this leaves the operational risk that a new component type needs to be added here as well.

If malicious host uses an invalid type, it can just as well use one of the valid component types, and use invalid fields for rest of values, right?

@cujomalainey cujomalainey Aug 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here was that components that were not expected to be in the module adapter (DAI) were being cast to process IPCs and resulting in out of bounds accesses. My opinion we have 2 problems here.

  1. we need to remove the legacy init types to simplify the logic.
  2. filter special types so we create a block list rather than an allow list.

@lyakh lyakh Aug 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding an enumeration value after all the valid DSP-bound (not testing ones) components and using

case SOF_COMP_NONE ... SOF_COMP_MAX:

?

@cujomalainey cujomalainey Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding an enumeration value after all the valid DSP-bound (not testing ones) components and using

case SOF_COMP_NONE ... SOF_COMP_MAX:

?

Doesn't work as *HOST and *DAI also caught in that range and that is intentionally not being caught here.

@cujomalainey cujomalainey Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is a copy from here

Which raises the question, are the missing COMPs also supposed to be here? If yes this further backs my suggestions in my previous comment to dissolve these hard coded types in the component layer.

@lyakh lyakh Aug 28, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work as *HOST and *DAI also caught in that range and that is intentionally not being caught here.

@cujomalainey ah, so they would go to default? But they aren't really "unsupported," they're just invalid for this operation? But ok, maybe would be good then to explicitly add them to the default case
as

case SOF_COMP_HOST:
case SOF_COMP_DAI:
default:
    ...

or at least mention that in a comment, but that's cosmetics, not really important.

@cujomalainey cujomalainey Aug 28, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend not adding the above to the default case. As much as it makes it exhaustive (which in something like rust would actually be important as it affects compilation) here it is cosmetic as you said so I would like the intention of added cases to be only for types that can be instantiated and not mix intents of support. Personal preference though.

{
const struct ipc_config_process *ipc_module_adapter = spec;

size = ipc_module_adapter->size;
data = ipc_module_adapter->data;
break;
}
default:
comp_err(dev, "module_adapter_init_data() unsupported comp type %d", config->type);
return -EINVAL;
}

/* Copy initial config */
Expand Down