ASoC: SOF: Fix buffer copy size error in ext bytes get and add checks #125

singalsu · 2018-09-11T12:46:13Z

This patch replaces the copy_to_user() size with the value that was
passed to the function minus the user data header length. It prevents
to exceed the user space buffer. The size originates from topology. The
compare to topology specified length is also added to ext bytes put.

The other changes add debug prints to kernel log to see the actual size
limits.

Signed-off-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com

ranj063 · 2018-09-11T21:30:07Z

sound/soc/sof/control.c

+	/* Limit size to prevent read of kernel data */
+	if (size > max_length) {
+		size = max_length;
+		dev_dbg(sdev->dev, "warning: the copy length was limited\n");


@singalsu Should this be an error? Does it make sense to copy partial data?

The size of copy originates from topology. I agree this should be better to be an error. There's need to fix topology if this happens.

ranj063 · 2018-09-11T21:30:48Z

sound/soc/sof/control.c

+		size = max_length;
+		dev_dbg(sdev->dev, "warning: the copy length was limited\n");
+	}
+
 	dev_dbg(sdev->dev, "getting data and command is %d\n", scontrol->cmd);


can we please remove this line? this was added for debug purposes when working on the demo.

ranj063 · 2018-09-12T01:10:07Z

sound/soc/sof/control.c

-	if (copy_to_user(binary_data + 2, cdata->data->data,
-			 SOF_IPC_MSG_MAX_SIZE - sizeof(struct sof_ipc_ctrl_data)))
+
+	if (copy_to_user(binary_data + 2, cdata->data->data, size))


@singalsu should this be "size" bytes or max_length bytes or the size sent by the FW?

The design problem with ASoC get is that the size should originate from firmware or at least have possibility to return the actual bytes read. Now the request is for the topology specified length but since it varies the request is always the max length. I used as example for this fix the similar ext bytes get function for Skylake.

But it should be acceptable for the user space to get a max length response and then use the uapi headers to get the relevant data. In EQ case the first uint32_t word after the user copy header is the length to use.

So, I'd keep this as is in the patch.

singalsu · 2018-09-12T10:33:32Z

@ranj063 I've updated the PR now (addressed 2 of 3). If you test this you will need to apply also PR #121. Without that the bytes get fails to work due to no data at DSP.

ranj063 · 2018-09-12T13:48:37Z

sound/soc/sof/control.c

+	if (err < 0)
+		dev_err(sdev->dev, "error: failed to idle %d\n", err);
+
+	return ret;


@singalsu I see why you have err and ret, but its confusing to see err just above and return "ret" instead. I think we should just use one and return the latest return value from the runtime_put. What do you think?

That's a good question. I need to find out if err means that the operation has failed with incorrect data or is this just something to note into debug trace. Failing with idle doesn't sound like the ext bytes get has failed. I never saw this to happen in my tests though.

There's also the return value from the IPC command that is not stored & checked. That might be the most relevant for return code.

@singalsu I think we should skip checking the return value for runtime_put if we arent going to use the value at all.
It is more important to check the return value for runtime_get_sync.

lgirdwood · 2018-09-13T07:48:11Z

sound/soc/sof/control.c

@@ -302,10 +302,13 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol,
 		return -EFAULT;

 	length = header[1];
-	dev_dbg(sdev->dev, "size of bytes put data is %d\n", length);
+	if (max_length > be->max)


We probably want a dev_warn() here stating that userspace is sending too much data and it has been truncated

lgirdwood · 2018-09-13T07:49:23Z

sound/soc/sof/control.c

+	 * the limit from ext bytes size from topology and SOF IPC
+	 * size limit.
+	 */
+	size -= 2 * sizeof(u32);


what is 2 * sizeof int32_t ?

@lgirdwood when sending the bytes from the userspace, the first 2 u32's are actually not part of the actual data to be sent to the firmware. Instead the first uint is a tag and the second is the size of bytes being sent. So we subtract those from the size to be passed to the firmware.
@singalsu can we please update the comment to reflect it and maybe define a macro like BYTE_CTL_HEADER_SIZE instead of 2?

Lets find the C structure that defines this ABI and subtract it's sizeof otherwise this code will break with any ABI update or on another architecture.

Yep, I'll search for struct.

I followed the example in sound/soc/intel/skylake/skl-topology.c function skl_tplg_tlv_control_get(). I'm wondering why the size need to be decremented. It prevents getting the max topology defined length of ext bytes data.

I searched in ASoC more examples and found one.

Function wm_coeff_tlv_get() in wm_adsp.c does not place the tag & size into header and does not decrement the size.

That way there would be no need for header struct knowledge but I haven't tested yet if that would work.

@ranj063 @lgirdwood Based on my quick test the ext bytes get works without placing the header to data[0] and data[1]. I can read back successfully the previously written coefficients to user space. Maybe using this header for get is a misunderstanding.

The header definitely is present in ext bytes put copy_from_user() so this looks different. Any thoughts on this?

@singalsu I would follow any prior art that is upstream on the matter. Lets do the same.

singalsu · 2018-09-21T12:06:50Z

@ranj063 @lgirdwood I've updated the PR now. Though I'm not sure if use of struct snd_ctl_tlv was your recommendation or should I use a SOF version of it. In line 385 I set the numid to zero. Also I wasn't sure about that vs. having macro in SOF. And it's not checked in bytes put. Thoughts?

lgirdwood · 2018-09-22T14:45:42Z

sound/soc/sof/control.c

+					SOF_IPC_COMP_GET_DATA,
+					SOF_CTRL_TYPE_DATA_GET, scontrol->cmd);
+
+	header.numid = 0;


Should't this be the same ID as passed in binary_data ?

Do you mean that numid should be set to scontrol->cmd (e.g. SOF_CTRL_CMD_BINARY)? And should we add check to put function that numid == scontrol->cmd?

This patch adds to bytes put a check to not exceed the topology defined bytes control length. The tlv control header is used for retrieving length instead to directly access the header words. In bytes get the size field in the header is set the to the value that is passed to the function minus the header length. It prevents to write past the user space buffer. The size parameter originates from topology. Checks for size violations vs. topology and IPC are added. The other changes add debug prints to kernel log to see the actual size limits and do code cleanup. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2018-09-24T14:34:29Z

@lgirdwood I just pushed updated version. The numid is now compared to/set to scontrol->cmd. Hope you meant this with your comment.

…part2 Documentation: teaching: labs: filesystems_part2: Fix typos

MAINTAINERS: update

…ed bind() Syzbot detected a NULL pointer dereference of nfc_llcp_sock->dev pointer (which is a 'struct nfc_dev *') with calls to llcp_sock_sendmsg() after a failed llcp_sock_bind(). The message being sent is a SOCK_DGRAM. KASAN report: BUG: KASAN: null-ptr-deref in nfc_alloc_send_skb+0x2d/0xc0 Read of size 4 at addr 00000000000005c8 by task llcp_sock_nfc_a/899 CPU: 5 PID: 899 Comm: llcp_sock_nfc_a Not tainted 5.16.0-rc6-next-20211224-00001-gc6437fbf18b0 thesofproject#125 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x45/0x59 ? nfc_alloc_send_skb+0x2d/0xc0 __kasan_report.cold+0x117/0x11c ? mark_lock+0x480/0x4f0 ? nfc_alloc_send_skb+0x2d/0xc0 kasan_report+0x38/0x50 nfc_alloc_send_skb+0x2d/0xc0 nfc_llcp_send_ui_frame+0x18c/0x2a0 ? nfc_llcp_send_i_frame+0x230/0x230 ? __local_bh_enable_ip+0x86/0xe0 ? llcp_sock_connect+0x470/0x470 ? llcp_sock_connect+0x470/0x470 sock_sendmsg+0x8e/0xa0 ____sys_sendmsg+0x253/0x3f0 ... The issue was visible only with multiple simultaneous calls to bind() and sendmsg(), which resulted in most of the bind() calls to fail. The bind() was failing on checking if there is available WKS/SDP/SAP (respective bit in 'struct nfc_llcp_local' fields). When there was no available WKS/SDP/SAP, the bind returned error but the sendmsg() to such socket was able to trigger mentioned NULL pointer dereference of nfc_llcp_sock->dev. The code looks simply racy and currently it protects several paths against race with checks for (!nfc_llcp_sock->local) which is NULL-ified in error paths of bind(). The llcp_sock_sendmsg() did not have such check but called function nfc_llcp_send_ui_frame() had, although not protected with lock_sock(). Therefore the race could look like (same socket is used all the time): CPU0 CPU1 ==== ==== llcp_sock_bind() - lock_sock() - success - release_sock() - return 0 llcp_sock_sendmsg() - lock_sock() - release_sock() llcp_sock_bind(), same socket - lock_sock() - error - nfc_llcp_send_ui_frame() - if (!llcp_sock->local) - llcp_sock->local = NULL - nfc_put_device(dev) - dereference llcp_sock->dev - release_sock() - return -ERRNO The nfc_llcp_send_ui_frame() checked llcp_sock->local outside of the lock, which is racy and ineffective check. Instead, its caller llcp_sock_sendmsg(), should perform the check inside lock_sock(). Reported-and-tested-by: syzbot+7f23bcddf626e0593a39@syzkaller.appspotmail.com Fixes: b874dec ("NFC: Implement LLCP connection less Tx path") Cc: <stable@vger.kernel.org> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Signed-off-by: David S. Miller <davem@davemloft.net>

Like commit 1cf3bfc ("bpf: Support 64-bit pointers to kfuncs") for s390x, add support for 64-bit pointers to kfuncs for LoongArch. Since the infrastructure is already implemented in BPF core, the only thing need to be done is to override bpf_jit_supports_far_kfunc_call(). Before this change, several test_verifier tests failed: # ./test_verifier | grep # | grep FAIL thesofproject#119/p calls: invalid kfunc call: ptr_to_mem to struct with non-scalar FAIL thesofproject#120/p calls: invalid kfunc call: ptr_to_mem to struct with nesting depth > 4 FAIL thesofproject#121/p calls: invalid kfunc call: ptr_to_mem to struct with FAM FAIL thesofproject#122/p calls: invalid kfunc call: reg->type != PTR_TO_CTX FAIL thesofproject#123/p calls: invalid kfunc call: void * not allowed in func proto without mem size arg FAIL thesofproject#124/p calls: trigger reg2btf_ids[reg->type] for reg->type > __BPF_REG_TYPE_MAX FAIL thesofproject#125/p calls: invalid kfunc call: reg->off must be zero when passed to release kfunc FAIL thesofproject#126/p calls: invalid kfunc call: don't match first member type when passed to release kfunc FAIL thesofproject#127/p calls: invalid kfunc call: PTR_TO_BTF_ID with negative offset FAIL thesofproject#128/p calls: invalid kfunc call: PTR_TO_BTF_ID with variable offset FAIL thesofproject#129/p calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID FAIL thesofproject#130/p calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID FAIL thesofproject#486/p map_kptr: ref: reference state created and released on xchg FAIL This is because the kfuncs in the loaded module are far away from __bpf_call_base: ffff800002009440 t bpf_kfunc_call_test_fail1 [bpf_testmod] 9000000002e128d8 T __bpf_call_base The offset relative to __bpf_call_base does NOT fit in s32, which breaks the assumption in BPF core. Enable bpf_jit_supports_far_kfunc_call() lifts this limit. Note that to reproduce the above result, tools/testing/selftests/bpf/config should be applied, and run the test with JIT enabled, unpriv BPF enabled. With this change, the test_verifier tests now all passed: # ./test_verifier ... Summary: 777 PASSED, 0 SKIPPED, 0 FAILED Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>

singalsu requested a review from ranj063 September 11, 2018 12:46

ranj063 reviewed Sep 11, 2018

View reviewed changes

ranj063 reviewed Sep 12, 2018

View reviewed changes

singalsu force-pushed the fix_ext_bytes_get_proposal branch from 7a64a66 to 171e225 Compare September 12, 2018 10:28

ranj063 reviewed Sep 12, 2018

View reviewed changes

singalsu requested a review from lgirdwood September 13, 2018 07:31

lgirdwood reviewed Sep 13, 2018

View reviewed changes

singalsu force-pushed the fix_ext_bytes_get_proposal branch from 171e225 to 0c7c77a Compare September 21, 2018 11:52

lgirdwood reviewed Sep 22, 2018

View reviewed changes

singalsu force-pushed the fix_ext_bytes_get_proposal branch from 0c7c77a to 36a50af Compare September 24, 2018 14:28

lgirdwood merged commit b5cde88 into thesofproject:topic/sof-dev Sep 24, 2018

paulstelian97 pushed a commit to paulstelian97/linux that referenced this pull request May 4, 2020

Merge pull request thesofproject#125 from Sergiu121/labs/filesystems_…

67b5176

…part2 Documentation: teaching: labs: filesystems_part2: Fix typos

aiChaoSONG pushed a commit to aiChaoSONG/linux that referenced this pull request May 6, 2021

Merge pull request thesofproject#125 from ojeda/maintainers

35b9d14

MAINTAINERS: update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: Fix buffer copy size error in ext bytes get and add checks #125

ASoC: SOF: Fix buffer copy size error in ext bytes get and add checks #125

singalsu commented Sep 11, 2018

ranj063 Sep 11, 2018

singalsu Sep 12, 2018

ranj063 Sep 11, 2018

singalsu Sep 12, 2018

ranj063 Sep 12, 2018

singalsu Sep 12, 2018

singalsu commented Sep 12, 2018

ranj063 Sep 12, 2018

singalsu Sep 12, 2018

ranj063 Sep 12, 2018

lgirdwood Sep 13, 2018

lgirdwood Sep 13, 2018

ranj063 Sep 18, 2018

lgirdwood Sep 18, 2018

singalsu Sep 18, 2018

singalsu Sep 18, 2018

singalsu Sep 18, 2018

lgirdwood Sep 18, 2018

singalsu commented Sep 21, 2018

lgirdwood Sep 22, 2018

singalsu Sep 24, 2018

singalsu commented Sep 24, 2018

ASoC: SOF: Fix buffer copy size error in ext bytes get and add checks #125

ASoC: SOF: Fix buffer copy size error in ext bytes get and add checks #125

Conversation

singalsu commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Sep 24, 2018