Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Ylk5o3EC/hyX5UQ0@google.com> <8eeb08ec-4836-cf7d-2285-8ed74ccfc1cb@linux.intel.com>
 <8986a1c6-b546-7a66-a778-048487624c95@linux.intel.com> <e31ff7c4-7cdc-fdaf-b12c-fc1e51798a1b@linux.intel.com>
In-Reply-To: <e31ff7c4-7cdc-fdaf-b12c-fc1e51798a1b@linux.intel.com>
From:   Curtis Malainey <cujomalainey@google.com>
Date:   Tue, 19 Apr 2022 11:04:33 -0700
Message-ID: <CAOReqxgRXbO3s4BgmZjoXmfKyr2MRWLE7jaTEwqh1ovgs_dUMA@mail.gmail.com>
Subject: Re: [Sound-open-firmware] out-of-bounds access in sound/soc/sof/topology.c
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     =?UTF-8?Q?P=C3=A9ter_Ujfalusi?= <peter.ujfalusi@linux.intel.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Jaska Uimonen <jaska.uimonen@linux.intel.com>,
        ALSA development <alsa-devel@alsa-project.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Takashi Iwai <tiwai@suse.com>,
        Tomasz Figa <tfiga@chromium.org>,
        Mark Brown <broonie@kernel.org>,
        Ricardo Ribalda <ribalda@chromium.org>,
        Sound Open Firmware <sound-open-firmware@alsa-project.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Apr 19, 2022 at 10:55 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 4/19/22 06:50, P=C3=A9ter Ujfalusi wrote:
> > Hi Sergey, Pierre,
> >
> > On 15/04/2022 19:00, Pierre-Louis Bossart wrote:
> >> 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 d=
efinitely
> >>> want to cherry pick the fix for -stable.
> >
> > I'm afraid, that this is still valid as of today, but in real life I
> > don't think it can happen.
> >
> >>> 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 cont=
rol */
> >>>
> >>>         /* 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 byte=
s
> >>> (`__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_ct=
rl_data).
> >>>
> >>> Now control data allocations looks as follows
> >>>
> >>>     scontrol->size =3D struct_size(scontrol->control_data, chanv,
> >>>                                  le32_to_cpu(mc->num_channels));
> >>>     scontrol->control_data =3D kzalloc(scontrol->size, GFP_KERNEL);
> >>>
> >>> Which is sizeof(sof_ipc_ctrl_data) + mc->num_channels * sizeof(sof_ip=
c_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 tha=
t
> >>>
> >>>    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 =3D wdata[i].control->control_data->data;
> >>>     *size +=3D wdata[i].pdata->size;
> >>>
> >>>
> >>> pdata->size is at offset 8, but if, say, mc->num_channels =3D=3D 1 th=
en
> >>> we allocate only 8 bytes for pdata, so pdata->size is 4 bytes outside
> >>> of allocated slab object.
> >>>
> >>> Thoughts?
> >
> > Your analyzes are spot on, unfortunately. But...
> >
> > As of today, the sof_get_control_data() is in the call path of
> > (ipc3-topology.c):
> >
> > sof_widget_update_ipc_comp_process() -> sof_process_load() ->
> > sof_get_control_data()
> >
> > sof_widget_update_ipc_comp_process() is the ipc_setup callback for
> > snd_soc_dapm_effect. If I'm not mistaken these only carries bin payload
> > and never MIXER/ENUM/SWITCH/VOLUME.
> > This means that the sof_get_control_data() is only called with
> > SND_SOC_TPLG_TYPE_BYTES and for that the allocated data area is correct=
.
> >
> > This can explain why we have not seen any issues so far. This does not
> > renders the code right, as how it is written atm is wrong.
>
>
> Sergey's results with KASAN show that there's a real-life problem though.=
 I also don't understand how that might happen.
>
> Could it be that these results are with a specific topology where our ass=
umptions are incorrect?
>

That would align with our testing as we are seeing the failing on
exactly once device with a custom topology with a bytes payload. See
sof-jsl-rt5682.m4 with -DWAVES configured for
sof-jsl-rt5682-rt1015.tplg