[PATCH 0/2] misc: fastrpc: Fixes for issues in userspace

[PATCH 0/2] misc: fastrpc: Fixes for issues in userspace

[Richard Acayan]

Hi everyone,

I've been playing around a bit with the FastRPC ioctl interface on the
Pixel 3a with some device tree patches. Since the ADSP crashes every 10
seconds, the inconvenience caused by related bugs encouraged me to write
a few fixes. A demonstration is provided in patch 2.

Please enjoy and review these patches for better userspace while remote
processors crash.

Richard Acayan (2):
  misc: fastrpc: return -EPIPE to invocations on device removal
  misc: fastrpc: reject new invocations during device removal

 drivers/misc/fastrpc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.39.1

[PATCH 1/2] misc: fastrpc: return -EPIPE to invocations on device removal

[Richard Acayan]

The return value is initialized as -1, or -EPERM. The completion of an
invocation implies that the return value is set appropriately, but
"Permission denied" does not accurately describe the outcome of the
invocation. Set the invocation's return value to a more appropriate
"Broken pipe", as the cleanup breaks the driver's connection with rpmsg.

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 drivers/misc/fastrpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7ccaca1b7cb8..2334a4fd5869 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2337,8 +2337,10 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
 	struct fastrpc_invoke_ctx *ctx;
 
 	spin_lock(&user->lock);
-	list_for_each_entry(ctx, &user->pending, node)
+	list_for_each_entry(ctx, &user->pending, node) {
+		ctx->retval = -EPIPE;
 		complete(&ctx->work);
+	}
 	spin_unlock(&user->lock);
 }
 
-- 
2.39.1

Re: [PATCH 1/2] misc: fastrpc: return -EPIPE to invocations on device removal

[Srinivas Kandagatla]

On 30/01/2023 22:27, Richard Acayan wrote:
> The return value is initialized as -1, or -EPERM. The completion of an
> invocation implies that the return value is set appropriately, but
> "Permission denied" does not accurately describe the outcome of the
> invocation. Set the invocation's return value to a more appropriate
> "Broken pipe", as the cleanup breaks the driver's connection with rpmsg.
> 
> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---

LGTM, thanks for fixing this,

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


--srini
>   drivers/misc/fastrpc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7ccaca1b7cb8..2334a4fd5869 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2337,8 +2337,10 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
>   	struct fastrpc_invoke_ctx *ctx;
>   
>   	spin_lock(&user->lock);
> -	list_for_each_entry(ctx, &user->pending, node)
> +	list_for_each_entry(ctx, &user->pending, node) {
> +		ctx->retval = -EPIPE;
>   		complete(&ctx->work);
> +	}
>   	spin_unlock(&user->lock);
>   }
>   

[PATCH 2/2] misc: fastrpc: reject new invocations during device removal

[Richard Acayan]

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.

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;
	}

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 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 2334a4fd5869..c8a36b9cf4fe 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.39.1

Re: [PATCH 2/2] misc: fastrpc: reject new invocations during device removal

[Srinivas Kandagatla]

On 30/01/2023 22:27, Richard Acayan wrote:
> 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.
> 

------------->cut<-------------
> 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;
> 	}
> 
------------->cut<-------------

move this after --- in commit log so that this is not part of commit log.

> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   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 2334a4fd5869..c8a36b9cf4fe 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
use /* */ style commenting, as its preferred one.

Once these are fixed


Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


--srini
>   	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);
>   }
>