diff mbox series

[[libubootenv] v2 3/3] handle protected mmcblk_boot_ devices

Message ID 20191009162934.20998-4-adrian.freihofer@siemens.com
State Changes Requested
Headers show
Series [[libubootenv] v2 3/3] handle protected mmcblk_boot_ devices | expand

Commit Message

Freihofer, Adrian Oct. 9, 2019, 4:29 p.m. UTC 
Some block devices support physical write protection. The kernel
provides a standard interface to enable or disable protection in
/sys/class/block/*/force_ro.

This patch adds functionality to automatically detect these memory
types. If read-only mode is enabled on the partition on which the
uboot environment must be written, libubootenv temporarily switches
to read/write mode.

Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
---
 src/CMakeLists.txt  |   2 +
 src/mmcblkboot_ro.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/mmcblkboot_ro.h |  11 +++++
 src/uboot_env.c     |  23 +++++++++
 4 files changed, 171 insertions(+)
 create mode 100644 src/mmcblkboot_ro.c
 create mode 100644 src/mmcblkboot_ro.h

Comments

Stefano Babic Oct. 10, 2019, 10:42 a.m. UTC | #1 
Hi Adrian,

On 09/10/19 18:29, Adrian Freihofer wrote:
> Some block devices support physical write protection. The kernel
> provides a standard interface to enable or disable protection in
> /sys/class/block/*/force_ro.
> 
> This patch adds functionality to automatically detect these memory
> types. If read-only mode is enabled on the partition on which the
> uboot environment must be written, libubootenv temporarily switches
> to read/write mode.
> 
> Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
> ---
>  src/CMakeLists.txt  |   2 +
>  src/mmcblkboot_ro.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/mmcblkboot_ro.h |  11 +++++
>  src/uboot_env.c     |  23 +++++++++
>  4 files changed, 171 insertions(+)
>  create mode 100644 src/mmcblkboot_ro.c
>  create mode 100644 src/mmcblkboot_ro.h
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 051732b..0b7aeaf 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -3,6 +3,8 @@ cmake_minimum_required (VERSION 2.6)
>  SET(libubootenv_SOURCES
>    uboot_env.c
>    uboot_private.h
> +  mmcblkboot_ro.c
> +  mmcblkboot_ro.h
>  )
>  
>  # Public headers
> diff --git a/src/mmcblkboot_ro.c b/src/mmcblkboot_ro.c
> new file mode 100644
> index 0000000..f9f768e
> --- /dev/null
> +++ b/src/mmcblkboot_ro.c
> @@ -0,0 +1,135 @@
> +/*
> + * SPDX-License-Identifier:     LGPL-2.1-or-later
> + *
> + * For some block device types, the uboot environment variables are stored in
> + * a read-only partition. The kernel provides an interface
> + * /sys/class/block/mmcblk?boot?/force_ro to enable and disable the write only
> + * flag.
> + *
> + * The reprotect function does not enable write protection if the memory was
> + * not protected before the unprotect function was called.
> + */
> +
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include "mmcblkboot_ro.h"
> +
> +typedef void (*fp_unprotect)(env_protect_mmcblkboot_t*);
> +typedef void (*fp_reprotect)(env_protect_mmcblkboot_t*);
> +
> +
> +#define SYSFS_PATH_MAX 120
> +
> +static const char c_sys_path_1[] = "/sys/class/block/";
> +static const char c_sys_path_2[] = "/force_ro";
> +static const char c_dev_name_1[] = "mmcblk";
> +static const char c_dev_name_2[] = "boot";
> +
> +/** mmcblk_boot device specific class */
> +typedef struct env_protect_mmcblkboot {
> +	fp_unprotect unprotect;
> +	fp_reprotect reprotect;
> +	char sysfs_path[SYSFS_PATH_MAX];
> +	char current_prot;
> +} env_protect_mmcblkboot_t;
> +
> +/** Enables read/write mode for the given device */
> +void mmcblkboot_unprotect(env_protect_mmcblkboot_t *p_obj) {
> +	const char c_unprot_char = '0';
> +	const char c_prot_char = '1';
> +	int fd;
> +	ssize_t n;
> +
> +	fd = open(p_obj->sysfs_path, O_RDWR);
> +	if (fd == -1) {
> +		return;
> +	}
> +
> +	// Verify and archive the current write protect state, unprotect the device
> +	n = read(fd, &(p_obj->current_prot), 1);
> +	if (n == 1 && (p_obj->current_prot == c_unprot_char || p_obj->current_prot == c_prot_char)) {
> +		write(fd, &c_unprot_char, 1);
> +	} else {
> +		p_obj->current_prot = 0; // undefined state
> +	}
> +	close(fd);
> +}
> +
> +/** re-activates the read only protection
> + *
> + * if it has been removed by the previous unprotect call.
> + *
> + */
> +void mmcblkboot_reprotect(env_protect_mmcblkboot_t *p_obj) {
> +	int fd;
> +
> +	if (p_obj->current_prot != 0) {
> +		fd = open(p_obj->sysfs_path, O_WRONLY);
> +		if (fd == -1) {
> +			return;
> +		}
> +		write(fd, &(p_obj->current_prot), 1);
> +		close(fd);
> +	}
> +}
> +
> +/**
> + * constructor of env_protect_mmcblkboot_t
> + *
> + * Creates an oject assigned to pp_obj if:
> + * - devname matches /dev/mmcblk[0-9]boot[0-9]
> + * - if a corresponding sysfs entry "force_ro" exists
> + * It returns 1 of the device needs to be unprotected, 0 if not. In case of an error,
> + * a negative error code is returned.
> + */
> +int mmcblkboot_create(env_protect_mmcblkboot_t **pp_obj, const char *devname) {
> +	env_protect_mmcblkboot_t *p_obj = NULL;
> +	const char *devfile = devname;
> +	int ret;
> +
> +	if (strncmp("/dev/", devname, 5) == 0) {
> +		devfile = devname + 5;
> +	} else {
> +		return 0;
> +	}
> +
> +	ret = strncmp(devfile, c_dev_name_1, sizeof(c_dev_name_1) - 1);
> +	if (ret != 0) {
> +		return 0;
> +	}
> +
> +	if (strncmp(devfile + sizeof(c_dev_name_1), c_dev_name_2, sizeof(c_dev_name_2) - 1) != 0) {
> +		return 0;
> +	}
> +
> +	if (*(devfile + sizeof(c_dev_name_1) - 1) < '0' ||
> +	    *(devfile + sizeof(c_dev_name_1) - 1) > '9') {
> +		return 0;
> +	}
> +
> +	if (*(devfile + sizeof(c_dev_name_1) + sizeof(c_dev_name_2) - 1) < '0' ||
> +	    *(devfile + sizeof(c_dev_name_1) + sizeof(c_dev_name_2) - 1) > '9') {
> +		return 0;
> +	}
> +
> +	p_obj = (env_protect_mmcblkboot_t *)calloc(1, sizeof(env_protect_mmcblkboot_t));
> +	if (p_obj == NULL) {
> +		return -ENOMEM;
> +	}
> +	snprintf(p_obj->sysfs_path, SYSFS_PATH_MAX, "%s%s%s", c_sys_path_1, devfile, c_sys_path_2);
> +
> +	if (access(p_obj->sysfs_path, W_OK) == -1) {
> +		free(p_obj);
> +		return 0;
> +	}
> +
> +	p_obj->unprotect = mmcblkboot_unprotect;
> +	p_obj->reprotect = mmcblkboot_reprotect;
> +
> +	*pp_obj = p_obj;
> +	return 1;
> +}
> diff --git a/src/mmcblkboot_ro.h b/src/mmcblkboot_ro.h
> new file mode 100644
> index 0000000..b86c7d7
> --- /dev/null
> +++ b/src/mmcblkboot_ro.h
> @@ -0,0 +1,11 @@
> +/*
> + * SPDX-License-Identifier:     LGPL-2.1-or-later
> + */
> +
> +#pragma once
> +
> +typedef struct env_protect_mmcblkboot env_protect_mmcblkboot_t;
> +
> +int mmcblkboot_create(env_protect_mmcblkboot_t **pp_obj, const char *devname);
> +void mmcblkboot_unprotect(env_protect_mmcblkboot_t *p_obj);
> +void mmcblkboot_reprotect(env_protect_mmcblkboot_t *p_obj);

Quite, but this does not guarantee the level of abstraction I will like
to get. The above code is ok, but the code in uboot_env should be
unaware of it and just call protect / unprotect function. Something like
(just to render the idea):

src/uboot_private.h:

struct uboot_flash_env {
  ...
         enum device_type        device_type;
	 typedef void (*fp_protect) (struct uboot_flash_env *dev, bool on);
 };

And in filewrite:

	if (dev->protect)
		dev->protect(dev, on);

	....

	if (dev->protect)
		dev->protect(dev, off);

So that this function is unaware if device supports it or not, just call it.

What do you mind ?

Regards,
Stefano

> diff --git a/src/uboot_env.c b/src/uboot_env.c
> index 726c433..4281468 100644
> --- a/src/uboot_env.c
> +++ b/src/uboot_env.c
> @@ -35,6 +35,7 @@
>  #include <mtd/ubi-user.h>
>  
>  #include "uboot_private.h"
> +#include "mmcblkboot_ro.h"
>  
>  #define DEVICE_MTD_NAME 		"/dev/mtd"
>  #define DEVICE_UBI_NAME 		"/dev/ubi"
> @@ -491,12 +492,34 @@ static int devread(struct uboot_ctx *ctx, unsigned int copy, void *data)
>  static int filewrite(struct uboot_flash_env *dev, void *data)
>  {
>  	int ret;
> +	env_protect_mmcblkboot_t *prot_handler = NULL;
> +
> +	// remove hardware write protection if needed
> +	ret = mmcblkboot_create(&prot_handler, dev->devname);

But if no block can be created (SD /SATA/..), the write is skipped.

> +	if(ret < 0) {
> +		goto filewrite_out;
> +	}
> +	if (prot_handler) {
> +		mmcblkboot_unprotect(prot_handler);
> +	}
>  
>  	if (dev->offset)
>  		lseek(dev->fd, dev->offset, SEEK_SET);
>  
>  	ret = write(dev->fd, data, dev->envsize);
>  
> +	// ensure all data are written before we might enable the read-only flag on the environment
> +	fsync(dev->fd);
> +
> +	// enforce hardware write protection again if it was active before this write
> +	if (prot_handler) {
> +		mmcblkboot_reprotect(prot_handler);
> +	}
> +
> +filewrite_out:
> +	if (prot_handler) {
> +		free(prot_handler);
> +	}
>  	return ret;
>  }
>  
>
diff mbox series

Patch

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 051732b..0b7aeaf 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -3,6 +3,8 @@  cmake_minimum_required (VERSION 2.6)
 SET(libubootenv_SOURCES
   uboot_env.c
   uboot_private.h
+  mmcblkboot_ro.c
+  mmcblkboot_ro.h
 )
 
 # Public headers
diff --git a/src/mmcblkboot_ro.c b/src/mmcblkboot_ro.c
new file mode 100644
index 0000000..f9f768e
--- /dev/null
+++ b/src/mmcblkboot_ro.c
@@ -0,0 +1,135 @@ 
+/*
+ * SPDX-License-Identifier:     LGPL-2.1-or-later
+ *
+ * For some block device types, the uboot environment variables are stored in
+ * a read-only partition. The kernel provides an interface
+ * /sys/class/block/mmcblk?boot?/force_ro to enable and disable the write only
+ * flag.
+ *
+ * The reprotect function does not enable write protection if the memory was
+ * not protected before the unprotect function was called.
+ */
+
+#include <stdlib.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+#include "mmcblkboot_ro.h"
+
+typedef void (*fp_unprotect)(env_protect_mmcblkboot_t*);
+typedef void (*fp_reprotect)(env_protect_mmcblkboot_t*);
+
+
+#define SYSFS_PATH_MAX 120
+
+static const char c_sys_path_1[] = "/sys/class/block/";
+static const char c_sys_path_2[] = "/force_ro";
+static const char c_dev_name_1[] = "mmcblk";
+static const char c_dev_name_2[] = "boot";
+
+/** mmcblk_boot device specific class */
+typedef struct env_protect_mmcblkboot {
+	fp_unprotect unprotect;
+	fp_reprotect reprotect;
+	char sysfs_path[SYSFS_PATH_MAX];
+	char current_prot;
+} env_protect_mmcblkboot_t;
+
+/** Enables read/write mode for the given device */
+void mmcblkboot_unprotect(env_protect_mmcblkboot_t *p_obj) {
+	const char c_unprot_char = '0';
+	const char c_prot_char = '1';
+	int fd;
+	ssize_t n;
+
+	fd = open(p_obj->sysfs_path, O_RDWR);
+	if (fd == -1) {
+		return;
+	}
+
+	// Verify and archive the current write protect state, unprotect the device
+	n = read(fd, &(p_obj->current_prot), 1);
+	if (n == 1 && (p_obj->current_prot == c_unprot_char || p_obj->current_prot == c_prot_char)) {
+		write(fd, &c_unprot_char, 1);
+	} else {
+		p_obj->current_prot = 0; // undefined state
+	}
+	close(fd);
+}
+
+/** re-activates the read only protection
+ *
+ * if it has been removed by the previous unprotect call.
+ *
+ */
+void mmcblkboot_reprotect(env_protect_mmcblkboot_t *p_obj) {
+	int fd;
+
+	if (p_obj->current_prot != 0) {
+		fd = open(p_obj->sysfs_path, O_WRONLY);
+		if (fd == -1) {
+			return;
+		}
+		write(fd, &(p_obj->current_prot), 1);
+		close(fd);
+	}
+}
+
+/**
+ * constructor of env_protect_mmcblkboot_t
+ *
+ * Creates an oject assigned to pp_obj if:
+ * - devname matches /dev/mmcblk[0-9]boot[0-9]
+ * - if a corresponding sysfs entry "force_ro" exists
+ * It returns 1 of the device needs to be unprotected, 0 if not. In case of an error,
+ * a negative error code is returned.
+ */
+int mmcblkboot_create(env_protect_mmcblkboot_t **pp_obj, const char *devname) {
+	env_protect_mmcblkboot_t *p_obj = NULL;
+	const char *devfile = devname;
+	int ret;
+
+	if (strncmp("/dev/", devname, 5) == 0) {
+		devfile = devname + 5;
+	} else {
+		return 0;
+	}
+
+	ret = strncmp(devfile, c_dev_name_1, sizeof(c_dev_name_1) - 1);
+	if (ret != 0) {
+		return 0;
+	}
+
+	if (strncmp(devfile + sizeof(c_dev_name_1), c_dev_name_2, sizeof(c_dev_name_2) - 1) != 0) {
+		return 0;
+	}
+
+	if (*(devfile + sizeof(c_dev_name_1) - 1) < '0' ||
+	    *(devfile + sizeof(c_dev_name_1) - 1) > '9') {
+		return 0;
+	}
+
+	if (*(devfile + sizeof(c_dev_name_1) + sizeof(c_dev_name_2) - 1) < '0' ||
+	    *(devfile + sizeof(c_dev_name_1) + sizeof(c_dev_name_2) - 1) > '9') {
+		return 0;
+	}
+
+	p_obj = (env_protect_mmcblkboot_t *)calloc(1, sizeof(env_protect_mmcblkboot_t));
+	if (p_obj == NULL) {
+		return -ENOMEM;
+	}
+	snprintf(p_obj->sysfs_path, SYSFS_PATH_MAX, "%s%s%s", c_sys_path_1, devfile, c_sys_path_2);
+
+	if (access(p_obj->sysfs_path, W_OK) == -1) {
+		free(p_obj);
+		return 0;
+	}
+
+	p_obj->unprotect = mmcblkboot_unprotect;
+	p_obj->reprotect = mmcblkboot_reprotect;
+
+	*pp_obj = p_obj;
+	return 1;
+}
diff --git a/src/mmcblkboot_ro.h b/src/mmcblkboot_ro.h
new file mode 100644
index 0000000..b86c7d7
--- /dev/null
+++ b/src/mmcblkboot_ro.h
@@ -0,0 +1,11 @@ 
+/*
+ * SPDX-License-Identifier:     LGPL-2.1-or-later
+ */
+
+#pragma once
+
+typedef struct env_protect_mmcblkboot env_protect_mmcblkboot_t;
+
+int mmcblkboot_create(env_protect_mmcblkboot_t **pp_obj, const char *devname);
+void mmcblkboot_unprotect(env_protect_mmcblkboot_t *p_obj);
+void mmcblkboot_reprotect(env_protect_mmcblkboot_t *p_obj);
diff --git a/src/uboot_env.c b/src/uboot_env.c
index 726c433..4281468 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -35,6 +35,7 @@ 
 #include <mtd/ubi-user.h>
 
 #include "uboot_private.h"
+#include "mmcblkboot_ro.h"
 
 #define DEVICE_MTD_NAME 		"/dev/mtd"
 #define DEVICE_UBI_NAME 		"/dev/ubi"
@@ -491,12 +492,34 @@  static int devread(struct uboot_ctx *ctx, unsigned int copy, void *data)
 static int filewrite(struct uboot_flash_env *dev, void *data)
 {
 	int ret;
+	env_protect_mmcblkboot_t *prot_handler = NULL;
+
+	// remove hardware write protection if needed
+	ret = mmcblkboot_create(&prot_handler, dev->devname);
+	if(ret < 0) {
+		goto filewrite_out;
+	}
+	if (prot_handler) {
+		mmcblkboot_unprotect(prot_handler);
+	}
 
 	if (dev->offset)
 		lseek(dev->fd, dev->offset, SEEK_SET);
 
 	ret = write(dev->fd, data, dev->envsize);
 
+	// ensure all data are written before we might enable the read-only flag on the environment
+	fsync(dev->fd);
+
+	// enforce hardware write protection again if it was active before this write
+	if (prot_handler) {
+		mmcblkboot_reprotect(prot_handler);
+	}
+
+filewrite_out:
+	if (prot_handler) {
+		free(prot_handler);
+	}
 	return ret;
 }