| Message ID | 20191021090620.11787-1-adrian.freihofer@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | raw_handler: handle ro block devices | expand |
Hi Adrian, On 21/10/19 11:06, 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. > There are a couple of wrong assumption in this patch: - rawimage is only for block device. This is not always true. - unprotect is just for SD / eMMC. Even not true: what about in case of SATA device, for example ? These items should be fixed. > Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com> > --- > handlers/raw_handler.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c > index ba87191..a14a8d2 100644 > --- a/handlers/raw_handler.c > +++ b/handlers/raw_handler.c > @@ -27,12 +27,99 @@ 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 matches /dev/mmcblk[0-9]boot[0-9] > + * - A corresponding ro flag e.g. /sys/class/block/mmcblk0boot0/force_ro is available > + * - The ro flag can be opened writeable > + */ > +static int blkprotect(struct img_type *img, bool on) > +{ > + const char c_sys_path_1[] = "/sys/class/block/"; > + const char c_sys_path_2[] = "/force_ro"; > + const char c_dev_name_1[] = "mmcblk"; > + const char c_dev_name_2[] = "boot"; > + const char c_unprot_char = '0'; > + const char c_prot_char = '1'; > + const char *devfile = img->device; > + int ret = 0; // 0 means OK nothing to do, 1 OK unprotected, negative means error > + int ret_int = 0; > + char *sysfs_path = NULL; > + int fd_force_ro; > + > + if (strncmp("/dev/", img->device, 5) == 0) { > + devfile = img->device + 5; > + } else { > + return ret; > + } > + Check is done in terms of filename. Even if this is correct in the most cases, it does not check really the type of the device. Even if this example is theoretical and built, what happens if I link /dev/mmcblk0 to /dev/ttyS0 ? Check looks weak. IMHO it should be done by calling lstat() on the device and check S_IFBLK on st.st_mode. > + ret_int = strncmp(devfile, c_dev_name_1, sizeof(c_dev_name_1) - 1); > + if (ret_int != 0) { > + return ret; > + } > + > + if (strncmp(devfile + sizeof(c_dev_name_1), c_dev_name_2, sizeof(c_dev_name_2) - 1) != 0) { > + return ret; > + } > + > + if (*(devfile + sizeof(c_dev_name_1) - 1) < '0' || > + *(devfile + sizeof(c_dev_name_1) - 1) > '9') { > + return ret; > + } > + > + 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 ret; > + } These checks seem weak and not required. If a device is a block device, the path is fixed and it is /sys/class/block/<device name/ro. > + > + ret_int = asprintf(&sysfs_path, "%s%s%s", c_sys_path_1, devfile, c_sys_path_2); > + 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; > + } > + > + if(on == false) { It is maybe easier to read if: 1. read current status 2. compare current with on 3. in case it differs, seek() and write to ro 4. close fd > + // Unprotect the device, if protected > + char current_prot; > + ssize_t n = read(fd_force_ro, ¤t_prot, 1); > + if (n == 1 && (current_prot == c_prot_char)) { This test is quite convolute. As I wrote before, this can be: ssize_t n = read(fd_force_ro, ¤t_prot, 1); if (n == 1 && is_set(current) != on) write(...); close(fd_force_ro); > + write(fd_force_ro, &c_unprot_char, 1); > + ret = 1; > + } > + } else { > + write(fd_force_ro, &c_prot_char, 1); > + } > + 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 +132,11 @@ static int install_raw_image(struct img_type *img, > ret = copyimage(&fdout, img, NULL); > #endif > > + if (prot_stat == 1) { > + fsync(fdout); // flush before ro mode gets applied Is this required ? Can you explain it ? Do you get error if you do not flush it ? > + blkprotect(img, true); // no error handling, keep ret from copyimage > + } > + > close(fdout); > return ret; > } > Best regards, Stefano Babic
diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c index ba87191..a14a8d2 100644 --- a/handlers/raw_handler.c +++ b/handlers/raw_handler.c @@ -27,12 +27,99 @@ 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 matches /dev/mmcblk[0-9]boot[0-9] + * - A corresponding ro flag e.g. /sys/class/block/mmcblk0boot0/force_ro is available + * - The ro flag can be opened writeable + */ +static int blkprotect(struct img_type *img, bool on) +{ + const char c_sys_path_1[] = "/sys/class/block/"; + const char c_sys_path_2[] = "/force_ro"; + const char c_dev_name_1[] = "mmcblk"; + const char c_dev_name_2[] = "boot"; + const char c_unprot_char = '0'; + const char c_prot_char = '1'; + const char *devfile = img->device; + int ret = 0; // 0 means OK nothing to do, 1 OK unprotected, negative means error + int ret_int = 0; + char *sysfs_path = NULL; + int fd_force_ro; + + if (strncmp("/dev/", img->device, 5) == 0) { + devfile = img->device + 5; + } else { + return ret; + } + + ret_int = strncmp(devfile, c_dev_name_1, sizeof(c_dev_name_1) - 1); + if (ret_int != 0) { + return ret; + } + + if (strncmp(devfile + sizeof(c_dev_name_1), c_dev_name_2, sizeof(c_dev_name_2) - 1) != 0) { + return ret; + } + + if (*(devfile + sizeof(c_dev_name_1) - 1) < '0' || + *(devfile + sizeof(c_dev_name_1) - 1) > '9') { + return ret; + } + + 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 ret; + } + + ret_int = asprintf(&sysfs_path, "%s%s%s", c_sys_path_1, devfile, c_sys_path_2); + 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; + } + + if(on == false) { + // Unprotect the device, if protected + char current_prot; + ssize_t n = read(fd_force_ro, ¤t_prot, 1); + if (n == 1 && (current_prot == c_prot_char)) { + write(fd_force_ro, &c_unprot_char, 1); + ret = 1; + } + } else { + write(fd_force_ro, &c_prot_char, 1); + } + 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 +132,11 @@ static int install_raw_image(struct img_type *img, ret = copyimage(&fdout, img, NULL); #endif + if (prot_stat == 1) { + fsync(fdout); // flush before ro mode gets applied + blkprotect(img, true); // no error handling, keep ret from copyimage + } + close(fdout); return ret; }
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 | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)