Thanks Sergey for this email. On 4/15/22 04:23, Sergey Senozhatsky wrote: > Hi, > > I'm running 5.10.111 LTS, so if this has been fixed already then we definitely > want to cherry pick the fix for -stable. > > > Anonymous union in this struct is of zero size > > /* generic control data */ > struct sof_ipc_ctrl_data { > struct sof_ipc_reply rhdr; > uint32_t comp_id; > > /* control access and data type */ > uint32_t type; /**< enum sof_ipc_ctrl_type */ > uint32_t cmd; /**< enum sof_ipc_ctrl_cmd */ > uint32_t index; /**< control index for comps > 1 control */ > > /* control data - can either be appended or DMAed from host */ > struct sof_ipc_host_buffer buffer; > uint32_t num_elems; /**< in array elems or bytes for data type */ > uint32_t elems_remaining; /**< elems remaining if sent in parts */ > > uint32_t msg_index; /**< for large messages sent in parts */ > > /* reserved for future use */ > uint32_t reserved[6]; > > /* control data - add new types if needed */ > union { > /* channel values can be used by volume type controls */ > struct sof_ipc_ctrl_value_chan chanv[0]; > /* component values used by routing controls like mux, mixer */ > struct sof_ipc_ctrl_value_comp compv[0]; > /* data can be used by binary controls */ > struct sof_abi_hdr data[0]; > }; > } __packed; > > sof_ipc_ctrl_value_chan and sof_ipc_ctrl_value_comp are of the same > size - 8 bytes, while sof_abi_hdr is much larger - _at least_ 32 bytes > (`__u32 data[0]` in sof_abi_hdr suggest that there should be more > payload after header). But they all contribute 0 to sizeof(sof_ipc_ctrl_data). > > Now control data allocations looks as follows > > scontrol->size = struct_size(scontrol->control_data, chanv, > le32_to_cpu(mc->num_channels)); > scontrol->control_data = kzalloc(scontrol->size, GFP_KERNEL); > > Which is sizeof(sof_ipc_ctrl_data) + mc->num_channels * sizeof(sof_ipc_ctrl_value_chan) > > For some reason it uses sizeof(sof_ipc_ctrl_value_chan), which is not > the largest member of the union. > > And this is where the problem is: in order to make control->data.FOO loads > and stores legal we need mc->num_channels to be of at least 4. So that > > sizeof(sof_ipc_ctrl_data) + mc->num_channels * sizeof(sof_ipc_ctrl_value_chan) > > 92 + 4 * 8 > > will be the same as > > sizeof(sof_ipc_ctrl_data) + sizeof(sof_abi_hdr). > > 92 + 32 > > Otherwise scontrol->control_data->data.FOO will access nearby/foreign > slab object. > > And there is at least one such memory access. In sof_get_control_data(). > > wdata[i].pdata = wdata[i].control->control_data->data; > *size += wdata[i].pdata->size; > > pdata->size is at offset 8, but if, say, mc->num_channels == 1 then > we allocate only 8 bytes for pdata, so pdata->size is 4 bytes outside > of allocated slab object. > > Thoughts? The SOF contributors who wrote that code are all on an extended Easter week-end or vacation so we'll need more time to provide a definitive answer. I am far from an expert on the topology, but I note that the 'data' field is only used for binary controls, where we use the maximum possible size for a control, without any arithmetic involving channels. It makes sense to me, the binary data does not have any defined structure, it's just a bunch of bytes provided as is to the firmware. static int sof_ipc3_control_load_bytes(struct snd_sof_dev *sdev, struct snd_sof_control *scontrol) { struct sof_ipc_ctrl_data *cdata; int ret; scontrol->ipc_control_data = kzalloc(scontrol->max_size, GFP_KERNEL); if (!scontrol->ipc_control_data) return -ENOMEM; In other cases, such as volumes and enums, the 'data' field doesn't seem to be used but we do use the channel information for volume and enums. static int sof_ipc3_control_load_volume(struct snd_sof_dev *sdev, struct snd_sof_control *scontrol) { struct sof_ipc_ctrl_data *cdata; int i; /* init the volume get/put data */ scontrol->size = struct_size(cdata, chanv, scontrol->num_channels); scontrol->ipc_control_data = kzalloc(scontrol->size, GFP_KERNEL); static int sof_ipc3_control_load_enum(struct snd_sof_dev *sdev, struct snd_sof_control *scontrol) { struct sof_ipc_ctrl_data *cdata; /* init the enum get/put data */ scontrol->size = struct_size(cdata, chanv, scontrol->num_channels); scontrol->ipc_control_data = kzalloc(scontrol->size, GFP_KERNEL); if (!scontrol->ipc_control_data) Given that we have two ways of allocating the memory, I am not sure there is a problem, but I could be wrong. I checked the v5.10.111 code and I see the same code, with the max_size being used for sof_control_load_bytes() and no channel-based arithmetic. Can I ask how you found out about this problem, is this the result of a warning/error reported by a software tool or based on your reviews of the code? Thanks -Pierre