Add checks to firmware IPC reply interpreter

[lyakh]

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]

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]

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

[lgirdwood]

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

[plbossart]

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

[cujomalainey]

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.