| Message ID | 20180629184818.GA37439@beast (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Fri, Jun 29, 2018 at 8:48 PM, Kees Cook <keescook@chromium.org> wrote: > In the quest to remove all stack VLA usage from the kernel[1], this > switches to using a kasprintf()ed buffer. Return paths are updated > to free the allocation. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 +++++-- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 28 +++++++++++++++++-------- > 2 files changed, 24 insertions(+), 11 deletions(-) This seems fine, though using a fixed-length string is probably just as well here, given that the 'fwname' variable is always set to the constant string "a530_zap.mdt" at the moment, which is not very long. Reviewed-by: Arnd Bergmann <arnd@arndb.de> Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 10:47:31PM +0200, Arnd Bergmann wrote: > On Fri, Jun 29, 2018 at 8:48 PM, Kees Cook <keescook@chromium.org> wrote: > > In the quest to remove all stack VLA usage from the kernel[1], this > > switches to using a kasprintf()ed buffer. Return paths are updated > > to free the allocation. > > > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 +++++-- > > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 28 +++++++++++++++++-------- > > 2 files changed, 24 insertions(+), 11 deletions(-) > > This seems fine, though using a fixed-length string is probably just > as well here, > given that the 'fwname' variable is always set to the constant string > "a530_zap.mdt" > at the moment, which is not very long. We could make it fixed but since this code is only called once this feels safer. Jordan
On Fri, Jun 29, 2018 at 11:48:18AM -0700, Kees Cook wrote: > In the quest to remove all stack VLA usage from the kernel[1], this > switches to using a kasprintf()ed buffer. Return paths are updated > to free the allocation. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Thanks for doing this. Acked-by: Jordan Crouse <jcrouse@codeaurora.org> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 +++++-- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 28 +++++++++++++++++-------- > 2 files changed, 24 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index d39400e5bc42..f5f76f8151f9 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -11,6 +11,7 @@ > * > */ > > +#include <linux/kernel.h> > #include <linux/types.h> > #include <linux/cpumask.h> > #include <linux/qcom_scm.h> > @@ -19,6 +20,7 @@ > #include <linux/soc/qcom/mdt_loader.h> > #include <linux/pm_opp.h> > #include <linux/nvmem-consumer.h> > +#include <linux/slab.h> > #include "msm_gem.h" > #include "msm_mmu.h" > #include "a5xx_gpu.h" > @@ -91,12 +93,13 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname) > ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, > mem_region, mem_phys, mem_size, NULL); > } else { > - char newname[strlen("qcom/") + strlen(fwname) + 1]; > + char *newname; > > - sprintf(newname, "qcom/%s", fwname); > + newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); > > ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID, > mem_region, mem_phys, mem_size, NULL); > + kfree(newname); > } > if (ret) > goto out; > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index bcbf9f2a29f9..bfaafdfc7eb2 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -17,7 +17,9 @@ > * this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/kernel.h> > #include <linux/pm_opp.h> > +#include <linux/slab.h> > #include "adreno_gpu.h" > #include "msm_gem.h" > #include "msm_mmu.h" > @@ -70,10 +72,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) > { > struct drm_device *drm = adreno_gpu->base.dev; > const struct firmware *fw = NULL; > - char newname[strlen("qcom/") + strlen(fwname) + 1]; > + char *newname; > int ret; > > - sprintf(newname, "qcom/%s", fwname); > + newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); > + if (!newname) > + return ERR_PTR(-ENOMEM); > > /* > * Try first to load from qcom/$fwfile using a direct load (to avoid > @@ -87,11 +91,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) > dev_info(drm->dev, "loaded %s from new location\n", > newname); > adreno_gpu->fwloc = FW_LOCATION_NEW; > - return fw; > + goto out; > } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { > dev_err(drm->dev, "failed to load %s: %d\n", > newname, ret); > - return ERR_PTR(ret); > + fw = ERR_PTR(ret); > + goto out; > } > } > > @@ -106,11 +111,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) > dev_info(drm->dev, "loaded %s from legacy location\n", > newname); > adreno_gpu->fwloc = FW_LOCATION_LEGACY; > - return fw; > + goto out; > } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { > dev_err(drm->dev, "failed to load %s: %d\n", > fwname, ret); > - return ERR_PTR(ret); > + fw = ERR_PTR(ret); > + goto out; > } > } > > @@ -126,16 +132,20 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) > dev_info(drm->dev, "loaded %s with helper\n", > newname); > adreno_gpu->fwloc = FW_LOCATION_HELPER; > - return fw; > + goto out; > } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { > dev_err(drm->dev, "failed to load %s: %d\n", > newname, ret); > - return ERR_PTR(ret); > + fw = ERR_PTR(ret); > + goto out; > } > } > > dev_err(drm->dev, "failed to load %s\n", fwname); > - return ERR_PTR(-ENOENT); > + fw = ERR_PTR(-ENOENT); > +out: > + kfree(newname); > + return fw; > } > > static int adreno_load_fw(struct adreno_gpu *adreno_gpu) > -- > 2.17.1 > > > -- > Kees Cook > Pixel Security
> @@ -91,12 +93,13 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname) > ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, > mem_region, mem_phys, mem_size, NULL); > } else { > - char newname[strlen("qcom/") + strlen(fwname) + 1]; > + char *newname; > > - sprintf(newname, "qcom/%s", fwname); > + newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); > > ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID, > mem_region, mem_phys, mem_size, NULL); I have taken another look also at this update suggestion. Now I wonder why the return value is not checked for the added name construction in the way as it is specified for the function “adreno_request_fw”. Will another condition check make sense at this place? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 2:20 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > On Fri, Jun 29, 2018 at 11:48:18AM -0700, Kees Cook wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this >> switches to using a kasprintf()ed buffer. Return paths are updated >> to free the allocation. >> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Thanks for doing this. > > Acked-by: Jordan Crouse <jcrouse@codeaurora.org> Hi Rob, Is this something you can take via your tree? Thanks, -Kees
On Thu, Aug 2, 2018 at 7:24 PM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Jun 29, 2018 at 2:20 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: >> On Fri, Jun 29, 2018 at 11:48:18AM -0700, Kees Cook wrote: >>> In the quest to remove all stack VLA usage from the kernel[1], this >>> switches to using a kasprintf()ed buffer. Return paths are updated >>> to free the allocation. >>> >>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com >> >> Thanks for doing this. >> >> Acked-by: Jordan Crouse <jcrouse@codeaurora.org> > > Hi Rob, > > Is this something you can take via your tree? > Oh, sorry, I missed this one, I'll pick it up shortly.. BR, -R -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index d39400e5bc42..f5f76f8151f9 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -11,6 +11,7 @@ * */ +#include <linux/kernel.h> #include <linux/types.h> #include <linux/cpumask.h> #include <linux/qcom_scm.h> @@ -19,6 +20,7 @@ #include <linux/soc/qcom/mdt_loader.h> #include <linux/pm_opp.h> #include <linux/nvmem-consumer.h> +#include <linux/slab.h> #include "msm_gem.h" #include "msm_mmu.h" #include "a5xx_gpu.h" @@ -91,12 +93,13 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname) ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys, mem_size, NULL); } else { - char newname[strlen("qcom/") + strlen(fwname) + 1]; + char *newname; - sprintf(newname, "qcom/%s", fwname); + newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID, mem_region, mem_phys, mem_size, NULL); + kfree(newname); } if (ret) goto out; diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index bcbf9f2a29f9..bfaafdfc7eb2 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -17,7 +17,9 @@ * this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/kernel.h> #include <linux/pm_opp.h> +#include <linux/slab.h> #include "adreno_gpu.h" #include "msm_gem.h" #include "msm_mmu.h" @@ -70,10 +72,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) { struct drm_device *drm = adreno_gpu->base.dev; const struct firmware *fw = NULL; - char newname[strlen("qcom/") + strlen(fwname) + 1]; + char *newname; int ret; - sprintf(newname, "qcom/%s", fwname); + newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); + if (!newname) + return ERR_PTR(-ENOMEM); /* * Try first to load from qcom/$fwfile using a direct load (to avoid @@ -87,11 +91,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) dev_info(drm->dev, "loaded %s from new location\n", newname); adreno_gpu->fwloc = FW_LOCATION_NEW; - return fw; + goto out; } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", newname, ret); - return ERR_PTR(ret); + fw = ERR_PTR(ret); + goto out; } } @@ -106,11 +111,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) dev_info(drm->dev, "loaded %s from legacy location\n", newname); adreno_gpu->fwloc = FW_LOCATION_LEGACY; - return fw; + goto out; } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", fwname, ret); - return ERR_PTR(ret); + fw = ERR_PTR(ret); + goto out; } } @@ -126,16 +132,20 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname) dev_info(drm->dev, "loaded %s with helper\n", newname); adreno_gpu->fwloc = FW_LOCATION_HELPER; - return fw; + goto out; } else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", newname, ret); - return ERR_PTR(ret); + fw = ERR_PTR(ret); + goto out; } } dev_err(drm->dev, "failed to load %s\n", fwname); - return ERR_PTR(-ENOENT); + fw = ERR_PTR(-ENOENT); +out: + kfree(newname); + return fw; } static int adreno_load_fw(struct adreno_gpu *adreno_gpu)
In the quest to remove all stack VLA usage from the kernel[1], this switches to using a kasprintf()ed buffer. Return paths are updated to free the allocation. [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 +++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 28 +++++++++++++++++-------- 2 files changed, 24 insertions(+), 11 deletions(-)