[PATCH v2 0/2] soc: qcom: rmtfs-mem: Support non-root rmtfs daemons

[PATCH v2 0/2] soc: qcom: rmtfs-mem: Support non-root rmtfs daemons

[Evan Green]

This series contains minor fixes needed to better support running an
rmtfs daemon from an unprivileged process:
 - Enable uevents on the child character device by adding a struct
class to the parent. I needed these so that I could change the
ownership of /dev/qcom_rmtfs_mem1, and drove myself crazy trying to
understand why my udev rules never fired.
 - Enable access to phys_addr and size sysfs attributes. The daemon
needs to read these, and they don't really contain anything sensitive,
so expose them.

I still need CAP_NET_ADMIN to be able to bind to the right qrtr port,
but at least with these changes I can run as a different user, and drop
all other privileges.

Changes in v2:
- Moved class registration/deregistration into init/exit routines (Brian)

Evan Green (2):
  soc: qcom: rmtfs-mem: Add class to enable uevents
  soc: qcom: rmtfs-mem: Make sysfs attributes world-readable

 drivers/soc/qcom/rmtfs_mem.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

-- 
2.18.1

[PATCH v2 1/2] soc: qcom: rmtfs-mem: Add class to enable uevents

[Evan Green]

Currently the qcom_rmtfs_memN devices are entirely invisible to the udev world.
Add a class to the rmtfs device so that uevents fire when the device is added.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v2:
- Moved class registration/deregistration into init/exit routines (Brian)

 drivers/soc/qcom/rmtfs_mem.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 97bb5989aa211..99a1363ece254 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -132,6 +132,11 @@ static int qcom_rmtfs_mem_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static struct class rmtfs_class = {
+	.owner          = THIS_MODULE,
+	.name           = "rmtfs",
+};
+
 static const struct file_operations qcom_rmtfs_mem_fops = {
 	.owner = THIS_MODULE,
 	.open = qcom_rmtfs_mem_open,
@@ -199,8 +204,8 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 
 	dev_set_name(&rmtfs_mem->dev, "qcom_rmtfs_mem%d", client_id);
 	rmtfs_mem->dev.id = client_id;
+	rmtfs_mem->dev.class = &rmtfs_class;
 	rmtfs_mem->dev.devt = MKDEV(MAJOR(qcom_rmtfs_mem_major), client_id);
-
 	ret = cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to add cdev: %d\n", ret);
@@ -239,7 +244,6 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
 put_device:
 	put_device(&rmtfs_mem->dev);
-
 	return ret;
 }
 
@@ -258,7 +262,6 @@ static int qcom_rmtfs_mem_remove(struct platform_device *pdev)
 
 	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
 	put_device(&rmtfs_mem->dev);
-
 	return 0;
 }
 
@@ -277,32 +280,42 @@ static struct platform_driver qcom_rmtfs_mem_driver = {
 	},
 };
 
-static int qcom_rmtfs_mem_init(void)
+static int __init qcom_rmtfs_mem_init(void)
 {
 	int ret;
 
+	ret = class_register(&rmtfs_class);
+	if (ret)
+		return ret;
+
 	ret = alloc_chrdev_region(&qcom_rmtfs_mem_major, 0,
 				  QCOM_RMTFS_MEM_DEV_MAX, "qcom_rmtfs_mem");
 	if (ret < 0) {
 		pr_err("qcom_rmtfs_mem: failed to allocate char dev region\n");
-		return ret;
+		goto unregister_class;
 	}
 
 	ret = platform_driver_register(&qcom_rmtfs_mem_driver);
 	if (ret < 0) {
 		pr_err("qcom_rmtfs_mem: failed to register rmtfs_mem driver\n");
-		unregister_chrdev_region(qcom_rmtfs_mem_major,
-					 QCOM_RMTFS_MEM_DEV_MAX);
+		goto unregister_chrdev;
 	}
 
+	return 0;
+
+unregister_chrdev:
+	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
+unregister_class:
+	class_unregister(&rmtfs_class);
 	return ret;
 }
 module_init(qcom_rmtfs_mem_init);
 
-static void qcom_rmtfs_mem_exit(void)
+static void __exit qcom_rmtfs_mem_exit(void)
 {
 	platform_driver_unregister(&qcom_rmtfs_mem_driver);
 	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
+	class_unregister(&rmtfs_class);
 }
 module_exit(qcom_rmtfs_mem_exit);
 
-- 
2.18.1

Re: [PATCH v2 1/2] soc: qcom: rmtfs-mem: Add class to enable uevents

[Brian Norris]

On Fri, Dec 21, 2018 at 12:10:00PM -0800, Evan Green wrote:
> Currently the qcom_rmtfs_memN devices are entirely invisible to the udev world.
> Add a class to the rmtfs device so that uevents fire when the device is added.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---

You've got some spurious whitespace changes below, but otherwise:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> Changes in v2:
> - Moved class registration/deregistration into init/exit routines (Brian)
> 
>  drivers/soc/qcom/rmtfs_mem.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 97bb5989aa211..99a1363ece254 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -132,6 +132,11 @@ static int qcom_rmtfs_mem_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static struct class rmtfs_class = {
> +	.owner          = THIS_MODULE,
> +	.name           = "rmtfs",
> +};
> +
>  static const struct file_operations qcom_rmtfs_mem_fops = {
>  	.owner = THIS_MODULE,
>  	.open = qcom_rmtfs_mem_open,
> @@ -199,8 +204,8 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  
>  	dev_set_name(&rmtfs_mem->dev, "qcom_rmtfs_mem%d", client_id);
>  	rmtfs_mem->dev.id = client_id;
> +	rmtfs_mem->dev.class = &rmtfs_class;
>  	rmtfs_mem->dev.devt = MKDEV(MAJOR(qcom_rmtfs_mem_major), client_id);
> -
>  	ret = cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to add cdev: %d\n", ret);
> @@ -239,7 +244,6 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  put_device:
>  	put_device(&rmtfs_mem->dev);
> -

Does this belong here?

>  	return ret;
>  }
>  
> @@ -258,7 +262,6 @@ static int qcom_rmtfs_mem_remove(struct platform_device *pdev)
>  
>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  	put_device(&rmtfs_mem->dev);
> -

And this?

Brian

>  	return 0;
>  }
>  
> @@ -277,32 +280,42 @@ static struct platform_driver qcom_rmtfs_mem_driver = {
>  	},
>  };
>  
> -static int qcom_rmtfs_mem_init(void)
> +static int __init qcom_rmtfs_mem_init(void)
>  {
>  	int ret;
>  
> +	ret = class_register(&rmtfs_class);
> +	if (ret)
> +		return ret;
> +
>  	ret = alloc_chrdev_region(&qcom_rmtfs_mem_major, 0,
>  				  QCOM_RMTFS_MEM_DEV_MAX, "qcom_rmtfs_mem");
>  	if (ret < 0) {
>  		pr_err("qcom_rmtfs_mem: failed to allocate char dev region\n");
> -		return ret;
> +		goto unregister_class;
>  	}
>  
>  	ret = platform_driver_register(&qcom_rmtfs_mem_driver);
>  	if (ret < 0) {
>  		pr_err("qcom_rmtfs_mem: failed to register rmtfs_mem driver\n");
> -		unregister_chrdev_region(qcom_rmtfs_mem_major,
> -					 QCOM_RMTFS_MEM_DEV_MAX);
> +		goto unregister_chrdev;
>  	}
>  
> +	return 0;
> +
> +unregister_chrdev:
> +	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
> +unregister_class:
> +	class_unregister(&rmtfs_class);
>  	return ret;
>  }
>  module_init(qcom_rmtfs_mem_init);
>  
> -static void qcom_rmtfs_mem_exit(void)
> +static void __exit qcom_rmtfs_mem_exit(void)
>  {
>  	platform_driver_unregister(&qcom_rmtfs_mem_driver);
>  	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
> +	class_unregister(&rmtfs_class);
>  }
>  module_exit(qcom_rmtfs_mem_exit);
>  
> -- 
> 2.18.1
> 

Re: [PATCH v2 1/2] soc: qcom: rmtfs-mem: Add class to enable uevents

[Bjorn Andersson]

On Fri 21 Dec 12:10 PST 2018, Evan Green wrote:

> Currently the qcom_rmtfs_memN devices are entirely invisible to the udev world.
> Add a class to the rmtfs device so that uevents fire when the device is added.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>

I agree with Brian on the whitespace chances, apart from that you have
my

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes in v2:
> - Moved class registration/deregistration into init/exit routines (Brian)
> 
>  drivers/soc/qcom/rmtfs_mem.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 97bb5989aa211..99a1363ece254 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -132,6 +132,11 @@ static int qcom_rmtfs_mem_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static struct class rmtfs_class = {
> +	.owner          = THIS_MODULE,
> +	.name           = "rmtfs",
> +};
> +
>  static const struct file_operations qcom_rmtfs_mem_fops = {
>  	.owner = THIS_MODULE,
>  	.open = qcom_rmtfs_mem_open,
> @@ -199,8 +204,8 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  
>  	dev_set_name(&rmtfs_mem->dev, "qcom_rmtfs_mem%d", client_id);
>  	rmtfs_mem->dev.id = client_id;
> +	rmtfs_mem->dev.class = &rmtfs_class;
>  	rmtfs_mem->dev.devt = MKDEV(MAJOR(qcom_rmtfs_mem_major), client_id);
> -
>  	ret = cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to add cdev: %d\n", ret);
> @@ -239,7 +244,6 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  put_device:
>  	put_device(&rmtfs_mem->dev);
> -
>  	return ret;
>  }
>  
> @@ -258,7 +262,6 @@ static int qcom_rmtfs_mem_remove(struct platform_device *pdev)
>  
>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>  	put_device(&rmtfs_mem->dev);
> -
>  	return 0;
>  }
>  
> @@ -277,32 +280,42 @@ static struct platform_driver qcom_rmtfs_mem_driver = {
>  	},
>  };
>  
> -static int qcom_rmtfs_mem_init(void)
> +static int __init qcom_rmtfs_mem_init(void)
>  {
>  	int ret;
>  
> +	ret = class_register(&rmtfs_class);
> +	if (ret)
> +		return ret;
> +
>  	ret = alloc_chrdev_region(&qcom_rmtfs_mem_major, 0,
>  				  QCOM_RMTFS_MEM_DEV_MAX, "qcom_rmtfs_mem");
>  	if (ret < 0) {
>  		pr_err("qcom_rmtfs_mem: failed to allocate char dev region\n");
> -		return ret;
> +		goto unregister_class;
>  	}
>  
>  	ret = platform_driver_register(&qcom_rmtfs_mem_driver);
>  	if (ret < 0) {
>  		pr_err("qcom_rmtfs_mem: failed to register rmtfs_mem driver\n");
> -		unregister_chrdev_region(qcom_rmtfs_mem_major,
> -					 QCOM_RMTFS_MEM_DEV_MAX);
> +		goto unregister_chrdev;
>  	}
>  
> +	return 0;
> +
> +unregister_chrdev:
> +	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
> +unregister_class:
> +	class_unregister(&rmtfs_class);
>  	return ret;
>  }
>  module_init(qcom_rmtfs_mem_init);
>  
> -static void qcom_rmtfs_mem_exit(void)
> +static void __exit qcom_rmtfs_mem_exit(void)
>  {
>  	platform_driver_unregister(&qcom_rmtfs_mem_driver);
>  	unregister_chrdev_region(qcom_rmtfs_mem_major, QCOM_RMTFS_MEM_DEV_MAX);
> +	class_unregister(&rmtfs_class);
>  }
>  module_exit(qcom_rmtfs_mem_exit);
>  
> -- 
> 2.18.1
> 

Re: [PATCH v2 1/2] soc: qcom: rmtfs-mem: Add class to enable uevents

[Evan Green]

On Fri, Dec 21, 2018 at 2:05 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 21 Dec 12:10 PST 2018, Evan Green wrote:
>
> > Currently the qcom_rmtfs_memN devices are entirely invisible to the udev world.
> > Add a class to the rmtfs device so that uevents fire when the device is added.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
>
> I agree with Brian on the whitespace chances, apart from that you have
> my
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>

Thanks Bjorn and Brian. I'll remove the whitespace changes and add
your reviewed by on the next spin.
-Evan

[PATCH v2 2/2] soc: qcom: rmtfs-mem: Make sysfs attributes world-readable

[Evan Green]

In order to run an rmtfs daemon as an unprivileged user, that user would
need access to the phys_addr and size sysfs attributes. Sharing these
attributes with unprivileged users doesn't really leak anything
sensitive, since if you have access to physical memory, the jig is
up anyway.

Make those attributes readable by all.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2: None

 drivers/soc/qcom/rmtfs_mem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 99a1363ece254..815c11aeceb93 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -45,9 +45,9 @@ static ssize_t qcom_rmtfs_mem_show(struct device *dev,
 			      struct device_attribute *attr,
 			      char *buf);
 
-static DEVICE_ATTR(phys_addr, 0400, qcom_rmtfs_mem_show, NULL);
-static DEVICE_ATTR(size, 0400, qcom_rmtfs_mem_show, NULL);
-static DEVICE_ATTR(client_id, 0400, qcom_rmtfs_mem_show, NULL);
+static DEVICE_ATTR(phys_addr, 0444, qcom_rmtfs_mem_show, NULL);
+static DEVICE_ATTR(size, 0444, qcom_rmtfs_mem_show, NULL);
+static DEVICE_ATTR(client_id, 0444, qcom_rmtfs_mem_show, NULL);
 
 static ssize_t qcom_rmtfs_mem_show(struct device *dev,
 			      struct device_attribute *attr,
-- 
2.18.1

Re: [PATCH v2 2/2] soc: qcom: rmtfs-mem: Make sysfs attributes world-readable

[Bjorn Andersson]

On Fri 21 Dec 12:10 PST 2018, Evan Green wrote:

> In order to run an rmtfs daemon as an unprivileged user, that user would
> need access to the phys_addr and size sysfs attributes. Sharing these
> attributes with unprivileged users doesn't really leak anything
> sensitive, since if you have access to physical memory, the jig is
> up anyway.
> 
> Make those attributes readable by all.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes in v2: None
> 
>  drivers/soc/qcom/rmtfs_mem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 99a1363ece254..815c11aeceb93 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -45,9 +45,9 @@ static ssize_t qcom_rmtfs_mem_show(struct device *dev,
>  			      struct device_attribute *attr,
>  			      char *buf);
>  
> -static DEVICE_ATTR(phys_addr, 0400, qcom_rmtfs_mem_show, NULL);
> -static DEVICE_ATTR(size, 0400, qcom_rmtfs_mem_show, NULL);
> -static DEVICE_ATTR(client_id, 0400, qcom_rmtfs_mem_show, NULL);
> +static DEVICE_ATTR(phys_addr, 0444, qcom_rmtfs_mem_show, NULL);
> +static DEVICE_ATTR(size, 0444, qcom_rmtfs_mem_show, NULL);
> +static DEVICE_ATTR(client_id, 0444, qcom_rmtfs_mem_show, NULL);
>  
>  static ssize_t qcom_rmtfs_mem_show(struct device *dev,
>  			      struct device_attribute *attr,
> -- 
> 2.18.1
>