Skip to content

dai-zephyr: fix use-after-free in dai copy #7660

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 2 commits into from
May 25, 2023
Merged

dai-zephyr: fix use-after-free in dai copy #7660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fffeb4
dai-zephyr: fix use-after-free in dai copy
kv2019i May 23, 2023
0e006ad
dai-zephyr: handle dynamic disconnect of one sink
kv2019i May 23, 2023
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
12 changes: 12 additions & 0 deletions src/audio/copier/copier.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,6 +1799,17 @@ static int copier_get_hw_params(struct comp_dev *dev, struct sof_ipc_stream_para
return dai_zephyr_get_hw_params(dd, dev, params, dir);
}

static int copier_unbind(struct comp_dev *dev, void *data)
{
struct copier_data *cd = comp_get_drvdata(dev);
struct dai_data *dd = cd->dd[0];

if (dev->ipc_config.type == SOF_COMP_DAI)
return dai_zephyr_unbind(dd, dev, data);

return 0;
}

static const struct comp_driver comp_copier = {
.uid = SOF_RT_UUID(copier_comp_uuid),
.tctx = &copier_comp_tr,
Expand All @@ -1820,6 +1831,7 @@ static const struct comp_driver comp_copier = {
.dai_ts_stop = copier_dai_ts_stop_op,
.dai_ts_get = copier_dai_ts_get_op,
.dai_get_hw_params = copier_get_hw_params,
.unbind = copier_unbind,
},
};

Expand Down
63 changes: 63 additions & 0 deletions src/audio/dai-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1461,6 +1461,37 @@ int dai_zephyr_multi_endpoint_copy(struct dai_data **dd, struct comp_dev *dev,
return 0;
}

static void set_new_local_buffer(struct dai_data *dd, struct comp_dev *dev)
{
struct comp_buffer __sparse_cache *dma_buf = buffer_acquire(dd->dma_buffer);
struct comp_buffer __sparse_cache *local_buf;
uint32_t dma_fmt = audio_stream_get_frm_fmt(&dma_buf->stream);
uint32_t local_fmt;

buffer_release(dma_buf);

if (dev->direction == SOF_IPC_STREAM_PLAYBACK)
dd->local_buffer = list_first_item(&dev->bsource_list,
struct comp_buffer,
sink_list);
else
dd->local_buffer = list_first_item(&dev->bsink_list,
struct comp_buffer,
source_list);

local_buf = buffer_acquire(dd->local_buffer);
local_fmt = audio_stream_get_frm_fmt(&local_buf->stream);
buffer_release(local_buf);

dd->process = pcm_get_conversion_function(local_fmt, dma_fmt);

if (!dd->process) {
comp_err(dev, "converter function NULL: local fmt %d dma fmt %d\n",
local_fmt, dma_fmt);
dd->local_buffer = NULL;
Copy link
Collaborator

@lyakh lyakh May 24, 2023

Choose a reason for hiding this comment

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

can we use a temporary variable in lines 1473-1480 above and only assign dd->local_buffer after this check?

Copy link
Collaborator Author

@kv2019i kv2019i May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh Took a look at this, but actually I'd prefer not to. We know at this point already that the old value of "dd->local_buffer" is invalid and will be soon freed from the heap, so we want to erase in any case. I.e. even if no converter is found, we want to reset dd->local_buffer.

Copy link
Collaborator

@lyakh lyakh May 25, 2023

Choose a reason for hiding this comment

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

sorry, maybe I didn't explain well. This function is only called if (!dd->local_buffer) right? Then it is assigned a nun-NULL value either in line 1474 or in line 1478 above. But then we check if we can find a conversion function and if not - set that pointer to NULL again. Whereas we could simply avoid setting it to something we cannot use anyway completely. That way at no point of time would it be set to something useless. I.e.

	struct comp_buffer *buf;
	...
	if (dev->direction == SOF_IPC_STREAM_PLAYBACK)
		buf = list_first_item(&dev->bsource_list,
						   struct comp_buffer,
						   sink_list);
	else
		buf = list_first_item(&dev->bsink_list,
						   struct comp_buffer,
						   source_list);

	local_buf = buffer_acquire(buf);
	local_fmt = audio_stream_get_frm_fmt(&local_buf->stream);
	buffer_release(local_buf);

	dd->process = pcm_get_conversion_function(local_fmt, dma_fmt);

	if (!dd->process)
		comp_err(dev, "converter function NULL: local fmt %d dma fmt %d\n",
			 local_fmt, dma_fmt);

	dd->local_buffer = buf;

Copy link
Collaborator Author

@kv2019i kv2019i May 25, 2023

Choose a reason for hiding this comment

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

Right, now I get your point @lyakh . Let me do a small fixup to this in separate PR given I got a green run on the CI and ok from reviewers and we can't of really need to have this fix asap in the tree.

Copy link
Collaborator Author

@kv2019i kv2019i May 25, 2023

Choose a reason for hiding this comment

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

-> #7690

}
}

/* copy and process stream data from source to sink buffers */
int dai_zephyr_copy(struct dai_data *dd, struct comp_dev *dev, pcm_converter_func *converter)
{
Expand Down Expand Up @@ -1504,6 +1535,15 @@ int dai_zephyr_copy(struct dai_data *dd, struct comp_dev *dev, pcm_converter_fun

buffer_release(buf_c);

/* handle module runtime unbind */
if (!dd->local_buffer) {
set_new_local_buffer(dd, dev);

if (!dd->local_buffer) {
comp_warn(dev, "dai_zephyr_copy(): local buffer unbound, cannot copy");
return 0;
}
}

/* calculate minimum size to copy */
if (dev->direction == SOF_IPC_STREAM_PLAYBACK) {
Expand Down Expand Up @@ -1743,6 +1783,29 @@ static uint64_t dai_get_processed_data(struct comp_dev *dev, uint32_t stream_no,
return ret;
}

#ifdef CONFIG_IPC_MAJOR_4
int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, void *data)
{
struct comp_buffer __sparse_cache *local_buf_c;
struct ipc4_module_bind_unbind *bu;
int buf_id;

bu = (struct ipc4_module_bind_unbind *)data;
buf_id = IPC4_COMP_ID(bu->extension.r.src_queue, bu->extension.r.dst_queue);

if (dd && dd->local_buffer) {
local_buf_c = buffer_acquire(dd->local_buffer);
if (local_buf_c->id == buf_id) {
comp_dbg(dev, "dai_zephyr_unbind: local_buffer %x unbound", buf_id);
dd->local_buffer = NULL;
}
buffer_release(local_buf_c);
}

return 0;
}
#endif /* CONFIG_IPC_MAJOR_4 */

static const struct comp_driver comp_dai = {
.type = SOF_COMP_DAI,
.uid = SOF_RT_UUID(dai_comp_uuid),
Expand Down
3 changes: 3 additions & 0 deletions src/include/sof/audio/dai_copier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,7 @@ int dai_zephyr_get_hw_params(struct dai_data *dd, struct comp_dev *dev,
int dai_zephyr_multi_endpoint_copy(struct dai_data **dd, struct comp_dev *dev,
struct comp_buffer *multi_endpoint_buffer,
int num_endpoints);

int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, void *data);

#endif /* __SOF_LIB_DAI_COPIER_H__ */