| Message ID | 20200820082843.s2jwxcqq53xtfon3@vctlabs.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix regression in UBI volume support | expand |
Hi S., On 20.08.20 10:28, S. Lockwood-Childs wrote: > Running realpath() directly on the name parsed from the env config file > fails in the special case of UBI volume syntax, e.g. > /dev/ubi0:env > ^^^------ UBI volume name > > That special case with UBI volume name is handled by ubi_update_name(), > which doesn't happen early enough for normalizing the device path. > With above example, libuboot_read_config() would attempt to normalize > "/dev/ubi0:env" which does not exist. > > Separate out the path normalization into a helper function that splits > off any volume part (e.g. ":env") before trying to normalize the path, > then appends the volume part to the normalized path. > > Signed-off-by: S. Lockwood-Childs <sjl@vctlabs.com> > --- > src/libuboot.h | 2 ++ > src/uboot_env.c | 57 +++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 52 insertions(+), 7 deletions(-) > > diff --git a/src/libuboot.h b/src/libuboot.h > index 06a2a3f..bfcaeb1 100644 > --- a/src/libuboot.h > +++ b/src/libuboot.h > @@ -15,6 +15,8 @@ struct uboot_ctx; > > #define DEVNAME_MAX_LENGTH 256 > > +#define DEVNAME_SEPARATOR ':' > + > /** Configuration passed in initialization > * > */ > diff --git a/src/uboot_env.c b/src/uboot_env.c > index 7aae6aa..3a51d26 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -268,7 +268,7 @@ static int ubi_update_name(struct uboot_flash_env *dev) > int dev_id, vol_id, ret = -EBADF; > char *sep; > > - sep = index(dev->devname, ':'); > + sep = index(dev->devname, DEVNAME_SEPARATOR); > if (sep) > { > memset(device, 0, DEVNAME_MAX_LENGTH); > @@ -294,6 +294,54 @@ out: > return ret; > } > > +static int normalize_device_path(char *path, struct uboot_flash_env *dev) > +{ > + char *sep = NULL, *normalized = NULL; > + size_t normalized_len = 0, volume_len = 0, output_len = 0; > + > + /* > + * if volume name is present, split into device path and volume > + * since only the device path needs normalized > + */ > + sep = index(path, DEVNAME_SEPARATOR); > + if (sep) > + { > + volume_len = strlen(sep); > + *sep = '\0'; > + } > + > + if ((normalized = realpath(path, NULL)) == NULL) > + { > + /* device file didn't exist */ > + return -EINVAL; > + } > + > + normalized_len = strlen(normalized); > + output_len = sizeof(dev->devname) - 1; /* leave room for null */ > + if ((normalized_len + volume_len) > output_len) > + { > + /* full name is too long to fit */ > + free(normalized); > + return -EINVAL; > + } > + > + /* > + * save normalized path to device file, > + * and possibly append separator char & volume name > + */ > + memset(dev->devname, 0, sizeof(dev->devname)); > + strncpy(dev->devname, normalized, output_len); > + free(normalized); > + > + if (sep) > + { > + *sep = DEVNAME_SEPARATOR; > + strncpy(dev->devname + normalized_len, sep, output_len - normalized_len); > + } > + > + return 0; > +} > + > static int check_env_device(struct uboot_ctx *ctx, struct uboot_flash_env *dev) > { > int fd, ret; > @@ -1115,7 +1163,6 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config) > int ndev = 0; > struct uboot_flash_env *dev; > char *tmp; > - char *path; > int retval = 0; > > if (!config) > @@ -1158,16 +1205,12 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config) > ctx->size = dev->envsize; > > if (tmp) { > - if ((path = realpath(tmp, NULL)) == NULL) { > + if (normalize_device_path(tmp, dev) < 0) { > free(tmp); > retval = -EINVAL; > break; > } > - > - memset(dev->devname, 0, sizeof(dev->devname)); > - strncpy(dev->devname, path, sizeof(dev->devname) - 1); > free(tmp); > - free(path); > } > > if (check_env_device(ctx, dev) < 0) { > Thanks for fixing this, it looks correct to me ! Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
diff --git a/src/libuboot.h b/src/libuboot.h index 06a2a3f..bfcaeb1 100644 --- a/src/libuboot.h +++ b/src/libuboot.h @@ -15,6 +15,8 @@ struct uboot_ctx; #define DEVNAME_MAX_LENGTH 256 +#define DEVNAME_SEPARATOR ':' + /** Configuration passed in initialization * */ diff --git a/src/uboot_env.c b/src/uboot_env.c index 7aae6aa..3a51d26 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -268,7 +268,7 @@ static int ubi_update_name(struct uboot_flash_env *dev) int dev_id, vol_id, ret = -EBADF; char *sep; - sep = index(dev->devname, ':'); + sep = index(dev->devname, DEVNAME_SEPARATOR); if (sep) { memset(device, 0, DEVNAME_MAX_LENGTH); @@ -294,6 +294,54 @@ out: return ret; } +static int normalize_device_path(char *path, struct uboot_flash_env *dev) +{ + char *sep = NULL, *normalized = NULL; + size_t normalized_len = 0, volume_len = 0, output_len = 0; + + /* + * if volume name is present, split into device path and volume + * since only the device path needs normalized + */ + sep = index(path, DEVNAME_SEPARATOR); + if (sep) + { + volume_len = strlen(sep); + *sep = '\0'; + } + + if ((normalized = realpath(path, NULL)) == NULL) + { + /* device file didn't exist */ + return -EINVAL; + } + + normalized_len = strlen(normalized); + output_len = sizeof(dev->devname) - 1; /* leave room for null */ + if ((normalized_len + volume_len) > output_len) + { + /* full name is too long to fit */ + free(normalized); + return -EINVAL; + } + + /* + * save normalized path to device file, + * and possibly append separator char & volume name + */ + memset(dev->devname, 0, sizeof(dev->devname)); + strncpy(dev->devname, normalized, output_len); + free(normalized); + + if (sep) + { + *sep = DEVNAME_SEPARATOR; + strncpy(dev->devname + normalized_len, sep, output_len - normalized_len); + } + + return 0; +} + static int check_env_device(struct uboot_ctx *ctx, struct uboot_flash_env *dev) { int fd, ret; @@ -1115,7 +1163,6 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config) int ndev = 0; struct uboot_flash_env *dev; char *tmp; - char *path; int retval = 0; if (!config) @@ -1158,16 +1205,12 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config) ctx->size = dev->envsize; if (tmp) { - if ((path = realpath(tmp, NULL)) == NULL) { + if (normalize_device_path(tmp, dev) < 0) { free(tmp); retval = -EINVAL; break; } - - memset(dev->devname, 0, sizeof(dev->devname)); - strncpy(dev->devname, path, sizeof(dev->devname) - 1); free(tmp); - free(path); } if (check_env_device(ctx, dev) < 0) {
Running realpath() directly on the name parsed from the env config file fails in the special case of UBI volume syntax, e.g. /dev/ubi0:env ^^^------ UBI volume name That special case with UBI volume name is handled by ubi_update_name(), which doesn't happen early enough for normalizing the device path. With above example, libuboot_read_config() would attempt to normalize "/dev/ubi0:env" which does not exist. Separate out the path normalization into a helper function that splits off any volume part (e.g. ":env") before trying to normalize the path, then appends the volume part to the normalized path. Signed-off-by: S. Lockwood-Childs <sjl@vctlabs.com> --- src/libuboot.h | 2 ++ src/uboot_env.c | 57 +++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 52 insertions(+), 7 deletions(-)