Re: [PATCH v5 10/10] drm/msm: Remove invalid overflow check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Changing the allocation size variable `sz` from `uint64_t` to `size_t` and removing the `sz > SIZE_MAX` check introduces a critical integer truncation vulnerability on 32-bit systems, leading to a massive heap buffer overflow.

Pre-existing issues:
- [High] Relocations allocated in `submit_lookup_cmds` are permanently leaked if an error occurs before `submit->nr_cmds` is initialized later in the ioctl.
--

--- Patch [10]: [PATCH v5 10/10] drm/msm: Remove invalid overflow check ---
commit eb92124b933ee21b339fcbad2bb72fb24329057d
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Remove invalid overflow check

This commit removes the overflow check for the allocation size in
submit_create(), noting that struct_size() saturates at SIZE_MAX and
kzalloc() already handles it. It also changes the type of the sz
variable from uint64_t to size_t.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 06b142d82e4d0..d914602fd443a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -36,15 +36,11 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  {
>  	static atomic_t ident = ATOMIC_INIT(0);
>  	struct msm_gem_submit *submit;
> -	uint64_t sz;
> +	size_t sz;
>  	int ret;
>  
>  	sz = struct_size(submit, bos, nr_bos) +
>  			((u64)nr_cmds * sizeof(submit->cmd[0]));
> -
> -	if (sz > SIZE_MAX)
> -		return ERR_PTR(-ENOMEM);
> -
>  	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);

[Severity: Critical]
Does this change introduce an integer truncation on 32-bit systems?

Since sz is now a size_t, the 64-bit sum of struct_size() and the
nr_cmds multiplication can be silently truncated to 32 bits before being
passed to kzalloc().

For example, if a large nr_cmds like 0x8000000 is provided, the added size
could be 0x100000000, which truncates to 0 in a 32-bit size_t.

Could this cause kzalloc() to succeed with a smaller size than required,
resulting in an out-of-bounds write when setting submit->cmd[i] later?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
msm_ioctl_gem_submit() leak the relocations?

Looking at submit_lookup_cmds(), memory is allocated for relocs:

    submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);

If an error occurs here or later in the ioctl, the code jumps to the error
path before submit->nr_cmds is initialized at the end of
msm_ioctl_gem_submit():

    submit->nr_cmds = args->nr_cmds;

Then __msm_gem_submit_destroy() will use the uninitialized submit->nr_cmds,
which is 0, for its cleanup loop:

    for (i = 0; i < submit->nr_cmds; i++)
        kfree(submit->cmd[i].relocs);

Could this result in permanently leaking all successfully allocated relocs
arrays if this error path is repeatedly triggered?

>  	if (!submit)
>  		return ERR_PTR(-ENOMEM);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=10