linux icon indicating copy to clipboard operation
linux copied to clipboard

[BUG] IPC ops do not check upper bounds on hw windows

Open cujomalainey opened this issue 3 years ago • 3 comments

Describe the bug IPC security issue, hdr.size is only bounded on lower end but not upper, resulting in potential memory access issue. If FW has been corrupted through a TLV control or similar, this could be used to reverse the attack into the kernel.

To Reproduce Code analysis

 /* DSP firmware has sent host a message  */                                                                                                                                                                                                                                                                               
 static void sof_ipc3_rx_msg(struct snd_sof_dev *sdev)                                                                                                                                                                                                                                                                     
 { 
        /* read back header */                               
        err = snd_sof_ipc_msg_data(sdev, NULL, &hdr, sizeof(hdr)); 
        if (err < 0) {
                dev_warn(sdev->dev, "failed to read IPC header: %d\n", err);
                return;                                                                                                                                        
        }                               
                                                                               
        if (hdr.size < sizeof(hdr)) {                                          <--- lower bound check
                dev_err(sdev->dev, "The received message size is invalid\n");
                return;                         
        }  
        
...

        /* read the full message */                                          
        msg_buf = kmalloc(hdr.size, GFP_KERNEL); <--- unbounded alloc
        if (!msg_buf)
                return;                                                        
                                                                               
        err = snd_sof_ipc_msg_data(sdev, NULL, msg_buf, hdr.size);  <--- unbounded read

e.g. Intel, but all platforms are susceptible (no size check anywhere)

int hda_ipc_msg_data(struct snd_sof_dev *sdev,
                     struct snd_pcm_substream *substream,
                     void *p, size_t sz)
{
        if (!substream || !sdev->stream_box.size) {
                sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
        } else {
                struct hdac_stream *hstream = substream->runtime->private_data; 
                struct sof_intel_hda_stream *hda_stream;

                hda_stream = container_of(hstream,
                                          struct sof_intel_hda_stream,
                                          hext_stream.hstream);

                /* The stream might already be closed */
                if (!hstream)
                        return -ESTRPIPE;

                sof_mailbox_read(sdev, hda_stream->sof_intel_stream.posn_offset, p, sz);
        }

        return 0;
}

Reproduction Rate N/A

Expected behavior Size is bounded to HW limits

Impact showstopper for security

Environment

  1. Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
    • Kernel: ToT

Screenshots or console output N/A

Please also include the relevant sections from the firmware log and kernel log in the report (and attach the full logs for complete reference). Kernel log is taken from dmesg and firmware log from sof-logger. See https://thesofproject.github.io/latest/developer_guides/debugability/logger/index.html

cujomalainey avatar Oct 04 '22 21:10 cujomalainey

IPC4 appears to be safe as it has fixed size read backs in sof_ipc4_rx_msg

cujomalainey avatar Oct 04 '22 21:10 cujomalainey

@ujfalusi this one is probably for you?

plbossart avatar Oct 05 '22 17:10 plbossart

reply_size also needs the same restrictions

cujomalainey avatar Oct 12 '22 01:10 cujomalainey

reply_size also needs the same restrictions

The reply size is protected in sof_ipc_tx_message():

	if (msg_bytes > ipc->max_payload_size ||
	    reply_bytes > ipc->max_payload_size)
		return -ENOBUFS;

ujfalusi avatar Dec 09 '22 10:12 ujfalusi