[v3,2/3] raw_handler: handle ro block devices
diff mbox series

Message ID 20191021203157.28524-3-adrian.freihofer@siemens.com
State Accepted
Headers show
Series
  • lstat instead of stat
Related show

Commit Message

Freihofer, Adrian Oct. 21, 2019, 8:31 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
image must be written, swupdate temporarily switches to read/write
mode.

Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
---
 handlers/raw_handler.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

Comments

Stefano Babic Oct. 22, 2019, 9:54 a.m. UTC | #1
On 21/10/19 22:31, 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
> image must be written, swupdate temporarily switches to read/write
> mode.
> 
> Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
> ---
>  handlers/raw_handler.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c
> index ba87191..6dfbe7d 100644
> --- a/handlers/raw_handler.c
> +++ b/handlers/raw_handler.c
> @@ -27,12 +27,91 @@ void raw_image_handler(void);
>  void raw_file_handler(void);
>  void raw_copyimage_handler(void);
>  
> +/**
> + * Handle write protection for block devices
> + *
> + * Automatically remove write protection for block devices if:
> + * - The device name starts with /dev/*
> + * - The device is a block device
> + * - A corresponding ro flag e.g. /sys/class/block/mmcblk0boot0/force_ro is available
> + * - The force_ro flag can be opened writeable
> + */
> +static int blkprotect(struct img_type *img, bool on)
> +{
> +	const char c_sys_path[] = "/sys/class/block/%s/force_ro";
> +	const char c_unprot_char = '0';
> +	const char c_prot_char = '1';
> +	int ret = 0;  // 0 means OK nothing to do, 1 OK unprotected, 2 OK protected, negative means error
> +	int ret_int = 0;
> +	char *sysfs_path = NULL;
> +	int fd_force_ro;
> +	struct stat sb;
> +	char current_prot;
> +
> +	if (strncmp("/dev/", img->device, 5) != 0) {
> +		return ret;
> +	}
> +
> +	if (lstat(img->device, &sb) == -1) {
> +		TRACE("stat for device %s failed: %s", img->device, strerror(errno));
> +		return ret;
> +	}
> +	if(!S_ISBLK(sb.st_mode)) {
> +		return ret;
> +	}
> +
> +	ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5);  // remove "/dev/" from device path
> +	if(ret_int < 0) {
> +		ret = -ENOMEM;
> +		goto blkprotect_out;
> +	}
> +
> +	if (access(sysfs_path, W_OK) == -1) {
> +		goto blkprotect_out;
> +	}
> +
> +	// There is a ro flag, the device needs to be protected or unprotected
> +	fd_force_ro = open(sysfs_path, O_RDWR);
> +	if (fd_force_ro == -1) {
> +		ret = -EBADF;
> +		goto blkprotect_out;
> +	}
> +
> +	ssize_t n = read(fd_force_ro, &current_prot, 1);
> +	if (n != 1) {
> +		ret = -EBADFD;
> +	}
> +	if (on == false) {
> +		if (current_prot == c_prot_char) {
> +			write(fd_force_ro, &c_unprot_char, 1);
> +			TRACE("Device %s: read-only protection disabled", img->device);
> +			ret = 1;
> +		}
> +	} else {
> +		if (current_prot == c_unprot_char) {
> +			write(fd_force_ro, &c_prot_char, 1);
> +			TRACE("Device %s: read-only protection enabled", img->device);
> +			ret = 2;
> +		}
> +	}
> +	close(fd_force_ro);
> +
> +blkprotect_out:
> +	if(sysfs_path)
> +		free(sysfs_path);
> +	return ret;
> +}
> +
>  static int install_raw_image(struct img_type *img,
>  	void __attribute__ ((__unused__)) *data)
>  {
>  	int ret;
>  	int fdout;
>  
> +	int prot_stat = blkprotect(img, false);
> +	if (prot_stat < 0)
> +		return prot_stat;
> +
>  	fdout = open(img->device, O_RDWR);
>  	if (fdout < 0) {
>  		TRACE("Device %s cannot be opened: %s",
> @@ -45,6 +124,11 @@ static int install_raw_image(struct img_type *img,
>  	ret = copyimage(&fdout, img, NULL);
>  #endif
>  
> +	if (prot_stat == 1) {
> +		fsync(fdout);  // At least with Linux 4.14 data are not automatically flushed before ro mode is enabled
> +		blkprotect(img, true);  // no error handling, keep ret from copyimage
> +	}
> +
>  	close(fdout);
>  	return ret;
>  }
> 

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic
Stefano Babic Oct. 22, 2019, 10:59 a.m. UTC | #2
Hi Adrian,

sorry, there is still an issue:

On 22/10/19 11:54, Stefano Babic wrote:
> On 21/10/19 22:31, 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
>> image must be written, swupdate temporarily switches to read/write
>> mode.
>>
>> Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
>> ---
>>  handlers/raw_handler.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>
>> diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c
>> index ba87191..6dfbe7d 100644
>> --- a/handlers/raw_handler.c
>> +++ b/handlers/raw_handler.c
>> @@ -27,12 +27,91 @@ void raw_image_handler(void);
>>  void raw_file_handler(void);
>>  void raw_copyimage_handler(void);
>>  
>> +/**
>> + * Handle write protection for block devices
>> + *
>> + * Automatically remove write protection for block devices if:
>> + * - The device name starts with /dev/*
>> + * - The device is a block device
>> + * - A corresponding ro flag e.g. /sys/class/block/mmcblk0boot0/force_ro is available
>> + * - The force_ro flag can be opened writeable
>> + */
>> +static int blkprotect(struct img_type *img, bool on)
>> +{
>> +	const char c_sys_path[] = "/sys/class/block/%s/force_ro";
>> +	const char c_unprot_char = '0';
>> +	const char c_prot_char = '1';
>> +	int ret = 0;  // 0 means OK nothing to do, 1 OK unprotected, 2 OK protected, negative means error
>> +	int ret_int = 0;
>> +	char *sysfs_path = NULL;
>> +	int fd_force_ro;
>> +	struct stat sb;
>> +	char current_prot;
>> +
>> +	if (strncmp("/dev/", img->device, 5) != 0) {
>> +		return ret;
>> +	}
>> +
>> +	if (lstat(img->device, &sb) == -1) {
>> +		TRACE("stat for device %s failed: %s", img->device, strerror(errno));
>> +		return ret;
>> +	}
>> +	if(!S_ISBLK(sb.st_mode)) {
>> +		return ret;
>> +	}
>> +
>> +	ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5);  // remove "/dev/" from device path
>> +	if(ret_int < 0) {
>> +		ret = -ENOMEM;
>> +		goto blkprotect_out;
>> +	}
>> +
>> +	if (access(sysfs_path, W_OK) == -1) {
>> +		goto blkprotect_out;
>> +	}
>> +
>> +	// There is a ro flag, the device needs to be protected or unprotected
>> +	fd_force_ro = open(sysfs_path, O_RDWR);
>> +	if (fd_force_ro == -1) {
>> +		ret = -EBADF;
>> +		goto blkprotect_out;
>> +	}
>> +
>> +	ssize_t n = read(fd_force_ro, &current_prot, 1);
>> +	if (n != 1) {
>> +		ret = -EBADFD;
>> +	}
>> +	if (on == false) {
>> +		if (current_prot == c_prot_char) {
>> +			write(fd_force_ro, &c_unprot_char, 1);

This generates a warning because the return value is not checked. Please
fix it, thanks.

I also thought about if this could be written in a more concise and
readable way. Instead of :

+	if (on == false) {
         ......

something like:

if (current_prot != (on ? c_prot_char : c_unprot_char) {
	ret=write(fd_force_ro, on ? &c_prot_char : &c_unprot_char, 1);
	..
}

What do you think ?

Best regards,
Stefano Babic

>> +			TRACE("Device %s: read-only protection disabled", img->device);
>> +			ret = 1;
>> +		}
>> +	} else {
>> +		if (current_prot == c_unprot_char) {
>> +			write(fd_force_ro, &c_prot_char, 1);

Ditto.

>> +			TRACE("Device %s: read-only protection enabled", img->device);
>> +			ret = 2;
>> +		}
>> +	}
>> +	close(fd_force_ro);
>> +
>> +blkprotect_out:
>> +	if(sysfs_path)
>> +		free(sysfs_path);
>> +	return ret;
>> +}
>> +
>>  static int install_raw_image(struct img_type *img,
>>  	void __attribute__ ((__unused__)) *data)
>>  {
>>  	int ret;
>>  	int fdout;
>>  
>> +	int prot_stat = blkprotect(img, false);
>> +	if (prot_stat < 0)
>> +		return prot_stat;
>> +
>>  	fdout = open(img->device, O_RDWR);
>>  	if (fdout < 0) {
>>  		TRACE("Device %s cannot be opened: %s",
>> @@ -45,6 +124,11 @@ static int install_raw_image(struct img_type *img,
>>  	ret = copyimage(&fdout, img, NULL);
>>  #endif
>>  
>> +	if (prot_stat == 1) {
>> +		fsync(fdout);  // At least with Linux 4.14 data are not automatically flushed before ro mode is enabled
>> +		blkprotect(img, true);  // no error handling, keep ret from copyimage
>> +	}
>> +
>>  	close(fdout);
>>  	return ret;
>>  }
>>
> 
> Acked-by: Stefano Babic <sbabic@denx.de>
> 
> Best regards,
> Stefano Babic
> 
>

Patch
diff mbox series

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c
index ba87191..6dfbe7d 100644
--- a/handlers/raw_handler.c
+++ b/handlers/raw_handler.c
@@ -27,12 +27,91 @@  void raw_image_handler(void);
 void raw_file_handler(void);
 void raw_copyimage_handler(void);
 
+/**
+ * Handle write protection for block devices
+ *
+ * Automatically remove write protection for block devices if:
+ * - The device name starts with /dev/*
+ * - The device is a block device
+ * - A corresponding ro flag e.g. /sys/class/block/mmcblk0boot0/force_ro is available
+ * - The force_ro flag can be opened writeable
+ */
+static int blkprotect(struct img_type *img, bool on)
+{
+	const char c_sys_path[] = "/sys/class/block/%s/force_ro";
+	const char c_unprot_char = '0';
+	const char c_prot_char = '1';
+	int ret = 0;  // 0 means OK nothing to do, 1 OK unprotected, 2 OK protected, negative means error
+	int ret_int = 0;
+	char *sysfs_path = NULL;
+	int fd_force_ro;
+	struct stat sb;
+	char current_prot;
+
+	if (strncmp("/dev/", img->device, 5) != 0) {
+		return ret;
+	}
+
+	if (lstat(img->device, &sb) == -1) {
+		TRACE("stat for device %s failed: %s", img->device, strerror(errno));
+		return ret;
+	}
+	if(!S_ISBLK(sb.st_mode)) {
+		return ret;
+	}
+
+	ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5);  // remove "/dev/" from device path
+	if(ret_int < 0) {
+		ret = -ENOMEM;
+		goto blkprotect_out;
+	}
+
+	if (access(sysfs_path, W_OK) == -1) {
+		goto blkprotect_out;
+	}
+
+	// There is a ro flag, the device needs to be protected or unprotected
+	fd_force_ro = open(sysfs_path, O_RDWR);
+	if (fd_force_ro == -1) {
+		ret = -EBADF;
+		goto blkprotect_out;
+	}
+
+	ssize_t n = read(fd_force_ro, &current_prot, 1);
+	if (n != 1) {
+		ret = -EBADFD;
+	}
+	if (on == false) {
+		if (current_prot == c_prot_char) {
+			write(fd_force_ro, &c_unprot_char, 1);
+			TRACE("Device %s: read-only protection disabled", img->device);
+			ret = 1;
+		}
+	} else {
+		if (current_prot == c_unprot_char) {
+			write(fd_force_ro, &c_prot_char, 1);
+			TRACE("Device %s: read-only protection enabled", img->device);
+			ret = 2;
+		}
+	}
+	close(fd_force_ro);
+
+blkprotect_out:
+	if(sysfs_path)
+		free(sysfs_path);
+	return ret;
+}
+
 static int install_raw_image(struct img_type *img,
 	void __attribute__ ((__unused__)) *data)
 {
 	int ret;
 	int fdout;
 
+	int prot_stat = blkprotect(img, false);
+	if (prot_stat < 0)
+		return prot_stat;
+
 	fdout = open(img->device, O_RDWR);
 	if (fdout < 0) {
 		TRACE("Device %s cannot be opened: %s",
@@ -45,6 +124,11 @@  static int install_raw_image(struct img_type *img,
 	ret = copyimage(&fdout, img, NULL);
 #endif
 
+	if (prot_stat == 1) {
+		fsync(fdout);  // At least with Linux 4.14 data are not automatically flushed before ro mode is enabled
+		blkprotect(img, true);  // no error handling, keep ret from copyimage
+	}
+
 	close(fdout);
 	return ret;
 }