| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACO55ttzvFZqUjXNut5n0ENR9e2+N2WfWnKnjfaWgNesspQ9UA@mail.gmail.com>
Date: Fri, 22 Jun 2018 19:50:18 +0200
From: Karol Herbst <kherbst@...hat.com>
To: Kees Cook <keescook@...omium.org>
Cc: linux-kernel@...r.kernel.org,
nouveau <nouveau@...ts.freedesktop.org>,
Alexandre Courbot <acourbot@...dia.com>,
dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Thu, May 24, 2018 at 7:24 PM, Kees Cook <keescook@...omium.org> wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> allocates the working buffers before starting the writing so it won't
> abort in the middle. This needs an initial walk of the lists to figure
> out how large the buffer should be.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> .../nouveau/nvkm/subdev/secboot/acr_r352.c | 25 ++++++++++++++++---
> .../nouveau/nvkm/subdev/secboot/acr_r367.c | 16 +++++++++++-
> 2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> index a721354249ce..d02e183717dc 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> @@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + /* Figure out how large we need gdesc to be. */
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> + acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
> struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
> const struct acr_r352_ls_func *ls_func =
> acr->func->ls_func[_img->falcon_id];
> - u8 gdesc[ls_func->bl_desc_size];
>
if there are no guarantees that (ls_func->bl_desc_size & 0x4 == 0),
then we need to memset a bit more, because 4 bytes at the time are
actually copied inside nvkm_gpuobj_memcpy_to later in that code, but
the last 4 bytes are only partly memset to 0.
If ls_func->bl_desc_size is always a multiple of 0x4, then it isn't as
important, but still better to be fixed. Or maybe
nvkm_gpuobj_memcpy_to should do that handling and check if the size is
a multiple of 0x4 and otherwise handle that case?
Same is valid for the changes in the r367 file.
> nvkm_gpuobj_memcpy_to(wpr_blob, pos, &img->wpr_header,
> sizeof(img->wpr_header));
> @@ -447,6 +460,8 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
>
> nvkm_done(wpr_blob);
>
> + kfree(gdesc);
> +
> return 0;
> }
>
> @@ -771,7 +786,11 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon *falcon,
> struct fw_bl_desc *hsbl_desc;
> void *bl, *blob_data, *hsbl_code, *hsbl_data;
> u32 code_size;
> - u8 bl_desc[bl_desc_size];
> + u8 *bl_desc;
> +
> + bl_desc = kzalloc(bl_desc_size, GFP_KERNEL);
> + if (!bl_desc)
> + return -ENOMEM;
>
> /* Find the bootloader descriptor for our blob and copy it */
> if (blob == acr->load_blob) {
> @@ -802,7 +821,6 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon *falcon,
> code_size, hsbl_desc->start_tag, 0, false);
>
> /* Generate the BL header */
> - memset(bl_desc, 0, bl_desc_size);
> acr->func->generate_hs_bl_desc(load_hdr, bl_desc, offset);
>
> /*
> @@ -811,6 +829,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon *falcon,
> nvkm_falcon_load_dmem(falcon, bl_desc, hsbl_desc->dmem_load_off,
> bl_desc_size, 0);
>
> + kfree(bl_desc);
> return hsbl_desc->start_tag << 8;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> index 866877b88797..978ad0790367 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> @@ -265,6 +265,19 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> + acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -272,7 +285,6 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
> struct ls_ucode_img_r367 *img = ls_ucode_img_r367(_img);
> const struct acr_r352_ls_func *ls_func =
> acr->func->ls_func[_img->falcon_id];
> - u8 gdesc[ls_func->bl_desc_size];
>
> nvkm_gpuobj_memcpy_to(wpr_blob, pos, &img->wpr_header,
> sizeof(img->wpr_header));
> @@ -298,6 +310,8 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct list_head *imgs,
>
> nvkm_done(wpr_blob);
>
> + kfree(gdesc);
> +
> return 0;
> }
>
> --
> 2.17.0
>
>
> --
> Kees Cook
> Pixel Security
> _______________________________________________
> Nouveau mailing list
> Nouveau@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
Powered by blists - more mailing lists