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=qPXydAacU1RqZW...
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)
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=qPXydAacU1RqZW...
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
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=qPXydAacU1RqZW...
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=qPXydAacU1RqZW...
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;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", newname, ret);goto out;
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;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", fwname, ret);goto out;
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;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) { dev_err(drm->dev, "failed to load %s: %d\n", newname, ret);goto out;
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
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=qPXydAacU1RqZW...
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=qPXydAacU1RqZW...
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
@@ -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
dri-devel@lists.freedesktop.org