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;
From:   Richard Acayan <mailingradian@gmail.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Amol Maheshwari <amahesh@qti.qualcomm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Richard Acayan <mailingradian@gmail.com>
Subject: [PATCH v2 2/2] misc: fastrpc: reject new invocations during device removal
Date:   Mon, 27 Mar 2023 17:02:18 -0400
Message-Id: <20230327210217.60948-3-mailingradian@gmail.com>
In-Reply-To: <20230327210217.60948-1-mailingradian@gmail.com>
References: <20230327210217.60948-1-mailingradian@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

The channel's rpmsg object allows new invocations to be made. After old
invocations are already interrupted, the driver shouldn't try to invoke
anymore. Invalidating the rpmsg at the end of the driver removal
function makes it easy to cause a race condition in userspace. Even
closing a file descriptor before the driver finishes its cleanup can
cause an invocation via fastrpc_release_current_dsp_process() and
subsequent timeout.

Invalidate the channel before the invocations are interrupted to make
sure that no invocations can be created to hang after the device closes.

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
Demonstration of the bug as performed on a Google Pixel 3a with
devicetree patches:

	#include <fcntl.h>
	#include <misc/fastrpc.h>
	#include <stdint.h>
	#include <stdio.h>
	#include <string.h>
	#include <sys/ioctl.h>
	#include <unistd.h>

	static int remotectl_open(int fd,
				  const char *name,
				  uint32_t *handle)
	{
		struct fastrpc_invoke invoke;
		struct fastrpc_invoke_args args[4];
		struct {
			uint32_t namelen;
			uint32_t errlen;
		} in;
		struct {
			uint32_t handle;
			uint32_t err;
		} out;
		char errstr[256];
		int ret;

		// Remoteproc expects to receive a null terminator
		in.namelen = strlen(name) + 1;
		in.errlen = 256;

		args[0].ptr = (__u64) &in;
		args[0].length = sizeof(in);
		args[0].fd = -1;

		args[1].ptr = (__u64) name;
		args[1].length = in.namelen;
		args[1].fd = -1;

		args[2].ptr = (__u64) &out;
		args[2].length = sizeof(out);
		args[2].fd = -1;

		args[3].ptr = (__u64) errstr;
		args[3].length = 256;
		args[3].fd = -1;

		invoke.handle = 0;
		invoke.sc = 0x00020200;
		invoke.args = (__u64) args;

		ret = ioctl(fd, FASTRPC_IOCTL_INVOKE, (__u64) &invoke);

		if (!ret)
			*handle = out.handle;

		return ret;
	}

	int main()
	{
		struct fastrpc_init_create_static create;
		uint32_t handle;
		int fd, ret;

		fd = open("/dev/fastrpc-adsp", O_RDWR);
		if (fd == -1) {
			perror("Could not open /dev/fastrpc-adsp");
			return 1;
		}

		ret = ioctl(fd, FASTRPC_IOCTL_INIT_ATTACH_SNS, NULL);
		if (ret) {
			perror("Could not attach to sensorspd");
			goto close_dev;
		}

		/*
		 * Under normal circumstances, the remote processor
		 * would request a file from a different client, and
		 * quickly find out that there is no such file. When
		 * this other client is not running, this procedure call
		 * conveniently waits for the ADSP to crash.
		 */
		ret = remotectl_open(fd, "a", &handle);
		if (ret == -1)
			perror("Could not open CHRE interface");

	close_dev:
		// This takes 10 seconds
		printf("Closing file descriptor\n");
		close(fd);
		printf("Closed file descriptor\n");

		return 0;
	}
---
 drivers/misc/fastrpc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 20c035af373a..f4116ce7805a 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2351,7 +2351,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	struct fastrpc_user *user;
 	unsigned long flags;
 
+	/* No invocations past this point */
 	spin_lock_irqsave(&cctx->lock, flags);
+	cctx->rpdev = NULL;
 	list_for_each_entry(user, &cctx->users, user)
 		fastrpc_notify_users(user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
@@ -2370,7 +2372,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 
 	of_platform_depopulate(&rpdev->dev);
 
-	cctx->rpdev = NULL;
 	fastrpc_channel_ctx_put(cctx);
 }
 
-- 
2.40.0