search

thesofproject / linux

Linux kernel source tree
Other
89 stars 129 forks source link

Add checks to firmware IPC reply interpreter #2096

Open lyakh opened 4 years ago

lyakh commented 4 years ago

Looking at snd_sof_fw_parse_ext_data() https://github.com/thesofproject/linux/blob/6da19674c8bf7ddf5af94c36b1d50a2ab5c35a29/sound/soc/sof/loader.c#L89 it parses firmware fw_ready() IPC message. While doing that it seems to unconditionally trust the correctness of data that has been read from the firmware, e.g.

        snd_sof_dsp_block_read(sdev, bar, offset + sizeof(*ext_hdr),
                   (void *)((u8 *)ext_data + sizeof(*ext_hdr)),
                   ext_hdr->hdr.size - sizeof(*ext_hdr));

which seems to be essentially copying ext_hdr->hdr.size bytes of data to a page-size allocated buffer without checking that size. Question: is it really ok to risk crashing the kernel in case of a firmware bug or should checks be added to this and similar code paths?

paulstelian97 commented 4 years ago

I'd say go light on checks. Do check but only the simplest checks that would otherwise cause kernel crashes. These checks shouldn't be too expensive though.

For example, do a bounds check on ext_hdr->hdr.size somewhere. Not necessarily here.

plbossart commented 3 years ago

Now that I re-read this, I don't think it's an enhancement but a bug with our implementation. @lgirdwood FYI

lgirdwood commented 3 years ago

This should never be bigger than PAGE_SIZE. @lyakh can you fix and validate this is less than PAGE_SIZE.

plbossart commented 1 year ago

this issue seems still relevant, even more so with @cujomalainey 's work to avoid trusting the firmware replies.

cujomalainey commented 1 year ago

Thanks for the ping. Yes if we get anything that interprets this, the fuzzer is going to have a field day with trusted fields. This will not pass our security requirements.