| Message ID | 1476193185-32107-2-git-send-email-matt.redfearn@imgtec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 10/11/2016 03:39 PM, Matt Redfearn wrote: > Storage of the firmware name was inconsistent, either storing a pointer > to a name stored with unknown ownership, or a variable length tacked > onto the end of the struct proc allocated in rproc_alloc. > > In preparation for allowing the firmware of an already allocated struct > rproc to be changed, the easiest way to allow reallocation of the name > is to switch to a fixed length buffer held as part of the struct rproc. > That way we can either copy the provided firmware name into it, or print > into it based on a name template. A new function, > rproc_set_firmware_name() is introduced for this purpose, and that logic > removed from rproc_alloc(). > > Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com> > --- > > drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++------------- > include/linux/remoteproc.h | 4 ++- > 2 files changed, 45 insertions(+), 23 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index fe0539ed9cb5..48cd9d5afb69 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev) > kfree(rproc); > } > > +/** > + * rproc_set_firmware_name() - helper to create a valid firmare name > + * @rproc: remote processor > + * @firmware: name of firmware file, can be NULL > + * > + * If the caller didn't pass in a firmware name then construct a default name, > + * otherwise the provided name is copied into the firmware field of struct > + * rproc. If the name is too long to fit, -EINVAL is returned. > + * > + * Returns 0 on success and an appropriate error code otherwise. > + */ > +static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware) > +{ > + char *cp, *template = "rproc-%s-fw"; > + int name_len; > + > + if (firmware) { > + name_len = strlen(firmware); > + cp = memchr(firmware, '\n', name_len); > + if (cp) > + name_len = cp - firmware; > + > + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN) Hi Matt, As you are added '\0' at offset name_len, name_len should be < to RPROC_MAX_FIRMWARE_NAME_LEN. Test should be if (name_len >= RPROC_MAX_FIRMWARE_NAME_LEN) Regards, Loic > + return -EINVAL; > + > + strncpy(rproc->firmware, firmware, name_len); > + rproc->firmware[name_len] = '\0'; > + } else { > + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN, > + template, rproc->name); > + } > + > + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware); > + return 0; > +} > + > static struct device_type rproc_type = { > .name = "remoteproc", > .release = rproc_type_release, > @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > const char *firmware, int len) > { > struct rproc *rproc; > - char *p, *template = "rproc-%s-fw"; > - int name_len = 0; > > if (!dev || !name || !ops) > return NULL; > > - if (!firmware) > - /* > - * Make room for default firmware name (minus %s plus '\0'). > - * If the caller didn't pass in a firmware name then > - * construct a default name. We're already glomming 'len' > - * bytes onto the end of the struct rproc allocation, so do > - * a few more for the default firmware name (but only if > - * the caller doesn't pass one). > - */ > - name_len = strlen(name) + strlen(template) - 2 + 1; > - > - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL); > + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > if (!rproc) > return NULL; > > - if (!firmware) { > - p = (char *)rproc + sizeof(struct rproc) + len; > - snprintf(p, name_len, template, name); > - } else { > - p = (char *)firmware; > - } > - > - rproc->firmware = p; > rproc->name = name; > rproc->ops = ops; > rproc->priv = &rproc[1]; > @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); > > + if (rproc_set_firmware_name(rproc, firmware)) { > + put_device(&rproc->dev); > + return NULL; > + } > + > atomic_set(&rproc->power, 0); > > /* Set ELF as the default fw_ops handler */ > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 1c457a8dd5a6..7a6f9ad55011 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -42,6 +42,8 @@ > #include <linux/idr.h> > #include <linux/of.h> > > +#define RPROC_MAX_FIRMWARE_NAME_LEN 128 > + > /** > * struct resource_table - firmware resource table header > * @ver: version number > @@ -416,7 +418,7 @@ struct rproc { > struct list_head node; > struct iommu_domain *domain; > const char *name; > - const char *firmware; > + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN]; > void *priv; > const struct rproc_ops *ops; > struct device dev; > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Loic, On 13/10/16 14:22, loic pallardy wrote: > > > On 10/11/2016 03:39 PM, Matt Redfearn wrote: >> Storage of the firmware name was inconsistent, either storing a pointer >> to a name stored with unknown ownership, or a variable length tacked >> onto the end of the struct proc allocated in rproc_alloc. >> >> In preparation for allowing the firmware of an already allocated struct >> rproc to be changed, the easiest way to allow reallocation of the name >> is to switch to a fixed length buffer held as part of the struct rproc. >> That way we can either copy the provided firmware name into it, or print >> into it based on a name template. A new function, >> rproc_set_firmware_name() is introduced for this purpose, and that logic >> removed from rproc_alloc(). >> >> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com> >> --- >> >> drivers/remoteproc/remoteproc_core.c | 64 >> +++++++++++++++++++++++------------- >> include/linux/remoteproc.h | 4 ++- >> 2 files changed, 45 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index fe0539ed9cb5..48cd9d5afb69 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device >> *dev) >> kfree(rproc); >> } >> >> +/** >> + * rproc_set_firmware_name() - helper to create a valid firmare name >> + * @rproc: remote processor >> + * @firmware: name of firmware file, can be NULL >> + * >> + * If the caller didn't pass in a firmware name then construct a >> default name, >> + * otherwise the provided name is copied into the firmware field of >> struct >> + * rproc. If the name is too long to fit, -EINVAL is returned. >> + * >> + * Returns 0 on success and an appropriate error code otherwise. >> + */ >> +static int rproc_set_firmware_name(struct rproc *rproc, const char >> *firmware) >> +{ >> + char *cp, *template = "rproc-%s-fw"; >> + int name_len; >> + >> + if (firmware) { >> + name_len = strlen(firmware); >> + cp = memchr(firmware, '\n', name_len); >> + if (cp) >> + name_len = cp - firmware; >> + >> + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN) > Hi Matt, > > As you are added '\0' at offset name_len, name_len should be < to > RPROC_MAX_FIRMWARE_NAME_LEN. > > Test should be if (name_len >= RPROC_MAX_FIRMWARE_NAME_LEN) Yes - good spot, thanks :-) Matt > > Regards, > Loic >> + return -EINVAL; >> + >> + strncpy(rproc->firmware, firmware, name_len); >> + rproc->firmware[name_len] = '\0'; >> + } else { >> + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN, >> + template, rproc->name); >> + } >> + >> + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware); >> + return 0; >> +} >> + >> static struct device_type rproc_type = { >> .name = "remoteproc", >> .release = rproc_type_release, >> @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, >> const char *name, >> const char *firmware, int len) >> { >> struct rproc *rproc; >> - char *p, *template = "rproc-%s-fw"; >> - int name_len = 0; >> >> if (!dev || !name || !ops) >> return NULL; >> >> - if (!firmware) >> - /* >> - * Make room for default firmware name (minus %s plus '\0'). >> - * If the caller didn't pass in a firmware name then >> - * construct a default name. We're already glomming 'len' >> - * bytes onto the end of the struct rproc allocation, so do >> - * a few more for the default firmware name (but only if >> - * the caller doesn't pass one). >> - */ >> - name_len = strlen(name) + strlen(template) - 2 + 1; >> - >> - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL); >> + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); >> if (!rproc) >> return NULL; >> >> - if (!firmware) { >> - p = (char *)rproc + sizeof(struct rproc) + len; >> - snprintf(p, name_len, template, name); >> - } else { >> - p = (char *)firmware; >> - } >> - >> - rproc->firmware = p; >> rproc->name = name; >> rproc->ops = ops; >> rproc->priv = &rproc[1]; >> @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, >> const char *name, >> >> dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); >> >> + if (rproc_set_firmware_name(rproc, firmware)) { >> + put_device(&rproc->dev); >> + return NULL; >> + } >> + >> atomic_set(&rproc->power, 0); >> >> /* Set ELF as the default fw_ops handler */ >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index 1c457a8dd5a6..7a6f9ad55011 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -42,6 +42,8 @@ >> #include <linux/idr.h> >> #include <linux/of.h> >> >> +#define RPROC_MAX_FIRMWARE_NAME_LEN 128 >> + >> /** >> * struct resource_table - firmware resource table header >> * @ver: version number >> @@ -416,7 +418,7 @@ struct rproc { >> struct list_head node; >> struct iommu_domain *domain; >> const char *name; >> - const char *firmware; >> + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN]; >> void *priv; >> const struct rproc_ops *ops; >> struct device dev; >> -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 11 Oct 06:39 PDT 2016, Matt Redfearn wrote: > Storage of the firmware name was inconsistent, either storing a pointer > to a name stored with unknown ownership, or a variable length tacked > onto the end of the struct proc allocated in rproc_alloc. > Instead of using a statically sized array for "firmware", just keep it a pointer to a local copy of the firmware name. > In preparation for allowing the firmware of an already allocated struct > rproc to be changed, the easiest way to allow reallocation of the name > is to switch to a fixed length buffer held as part of the struct rproc. > That way we can either copy the provided firmware name into it, or print > into it based on a name template. A new function, > rproc_set_firmware_name() is introduced for this purpose, and that logic > removed from rproc_alloc(). > > Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com> > --- > > drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++------------- > include/linux/remoteproc.h | 4 ++- > 2 files changed, 45 insertions(+), 23 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index fe0539ed9cb5..48cd9d5afb69 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev) > kfree(rproc); > } > > +/** > + * rproc_set_firmware_name() - helper to create a valid firmare name > + * @rproc: remote processor > + * @firmware: name of firmware file, can be NULL > + * > + * If the caller didn't pass in a firmware name then construct a default name, > + * otherwise the provided name is copied into the firmware field of struct > + * rproc. If the name is too long to fit, -EINVAL is returned. > + * > + * Returns 0 on success and an appropriate error code otherwise. > + */ > +static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware) That would turn this function into something like: char *p, *template = "rproc-%s-fw"; if (!firmware) { name_len = strlen(name) + strlen(template) - 2 + 1; p = kmalloc(name_len, GFP_KERNEL); if (!p) return -ENOMEM; sprintf(p, template, firmware); } else { p = kstrdup(firmware, GFP_KERNEL); if (!p) return -ENOMEM; } kfree(rproc->firmware); rproc->firmware = p; Like your version this leaves the firmware name unchanged in the case of an error. > +{ > + char *cp, *template = "rproc-%s-fw"; > + int name_len; > + > + if (firmware) { > + name_len = strlen(firmware); > + cp = memchr(firmware, '\n', name_len); Let's just require the caller to pass us a valid name. > + if (cp) > + name_len = cp - firmware; > + > + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN) > + return -EINVAL; > + > + strncpy(rproc->firmware, firmware, name_len); > + rproc->firmware[name_len] = '\0'; > + } else { > + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN, > + template, rproc->name); > + } > + > + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware); > + return 0; > +} PS. Don't forget to free the string in rproc_type_release() Regards, Bjorn > + > static struct device_type rproc_type = { > .name = "remoteproc", > .release = rproc_type_release, > @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > const char *firmware, int len) > { > struct rproc *rproc; > - char *p, *template = "rproc-%s-fw"; > - int name_len = 0; > > if (!dev || !name || !ops) > return NULL; > > - if (!firmware) > - /* > - * Make room for default firmware name (minus %s plus '\0'). > - * If the caller didn't pass in a firmware name then > - * construct a default name. We're already glomming 'len' > - * bytes onto the end of the struct rproc allocation, so do > - * a few more for the default firmware name (but only if > - * the caller doesn't pass one). > - */ > - name_len = strlen(name) + strlen(template) - 2 + 1; > - > - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL); > + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > if (!rproc) > return NULL; > > - if (!firmware) { > - p = (char *)rproc + sizeof(struct rproc) + len; > - snprintf(p, name_len, template, name); > - } else { > - p = (char *)firmware; > - } > - > - rproc->firmware = p; > rproc->name = name; > rproc->ops = ops; > rproc->priv = &rproc[1]; > @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); > > + if (rproc_set_firmware_name(rproc, firmware)) { > + put_device(&rproc->dev); > + return NULL; > + } > + > atomic_set(&rproc->power, 0); > > /* Set ELF as the default fw_ops handler */ > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 1c457a8dd5a6..7a6f9ad55011 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -42,6 +42,8 @@ > #include <linux/idr.h> > #include <linux/of.h> > > +#define RPROC_MAX_FIRMWARE_NAME_LEN 128 > + > /** > * struct resource_table - firmware resource table header > * @ver: version number > @@ -416,7 +418,7 @@ struct rproc { > struct list_head node; > struct iommu_domain *domain; > const char *name; > - const char *firmware; > + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN]; > void *priv; > const struct rproc_ops *ops; > struct device dev; > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index fe0539ed9cb5..48cd9d5afb69 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev) kfree(rproc); } +/** + * rproc_set_firmware_name() - helper to create a valid firmare name + * @rproc: remote processor + * @firmware: name of firmware file, can be NULL + * + * If the caller didn't pass in a firmware name then construct a default name, + * otherwise the provided name is copied into the firmware field of struct + * rproc. If the name is too long to fit, -EINVAL is returned. + * + * Returns 0 on success and an appropriate error code otherwise. + */ +static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware) +{ + char *cp, *template = "rproc-%s-fw"; + int name_len; + + if (firmware) { + name_len = strlen(firmware); + cp = memchr(firmware, '\n', name_len); + if (cp) + name_len = cp - firmware; + + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN) + return -EINVAL; + + strncpy(rproc->firmware, firmware, name_len); + rproc->firmware[name_len] = '\0'; + } else { + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN, + template, rproc->name); + } + + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware); + return 0; +} + static struct device_type rproc_type = { .name = "remoteproc", .release = rproc_type_release, @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, const char *firmware, int len) { struct rproc *rproc; - char *p, *template = "rproc-%s-fw"; - int name_len = 0; if (!dev || !name || !ops) return NULL; - if (!firmware) - /* - * Make room for default firmware name (minus %s plus '\0'). - * If the caller didn't pass in a firmware name then - * construct a default name. We're already glomming 'len' - * bytes onto the end of the struct rproc allocation, so do - * a few more for the default firmware name (but only if - * the caller doesn't pass one). - */ - name_len = strlen(name) + strlen(template) - 2 + 1; - - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL); + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); if (!rproc) return NULL; - if (!firmware) { - p = (char *)rproc + sizeof(struct rproc) + len; - snprintf(p, name_len, template, name); - } else { - p = (char *)firmware; - } - - rproc->firmware = p; rproc->name = name; rproc->ops = ops; rproc->priv = &rproc[1]; @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); + if (rproc_set_firmware_name(rproc, firmware)) { + put_device(&rproc->dev); + return NULL; + } + atomic_set(&rproc->power, 0); /* Set ELF as the default fw_ops handler */ diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 1c457a8dd5a6..7a6f9ad55011 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -42,6 +42,8 @@ #include <linux/idr.h> #include <linux/of.h> +#define RPROC_MAX_FIRMWARE_NAME_LEN 128 + /** * struct resource_table - firmware resource table header * @ver: version number @@ -416,7 +418,7 @@ struct rproc { struct list_head node; struct iommu_domain *domain; const char *name; - const char *firmware; + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN]; void *priv; const struct rproc_ops *ops; struct device dev;
Storage of the firmware name was inconsistent, either storing a pointer to a name stored with unknown ownership, or a variable length tacked onto the end of the struct proc allocated in rproc_alloc. In preparation for allowing the firmware of an already allocated struct rproc to be changed, the easiest way to allow reallocation of the name is to switch to a fixed length buffer held as part of the struct rproc. That way we can either copy the provided firmware name into it, or print into it based on a name template. A new function, rproc_set_firmware_name() is introduced for this purpose, and that logic removed from rproc_alloc(). Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com> --- drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++------------- include/linux/remoteproc.h | 4 ++- 2 files changed, 45 insertions(+), 23 deletions(-)