On Fri, Jun 22, 2018 at 10:50 AM, Karol Herbst <kherbst@xxxxxxxxxx> wrote: > On Thu, May 24, 2018 at 7:24 PM, Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx >> >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> >> --- >> .../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. I think this is unchanged from the original code, yes? The memset() is always against bl_desc_size; I haven't changed that. > 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. Should I resend with both the allocation and the memset getting rounded up to the next multiple of 4? -Kees -- Kees Cook Pixel Security _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
