linux-kernel - Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Mar 2022 11:21:21 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Akhil P Oommen <quic_akhilpo@...cinc.com>,
        Jonathan Marek <jonathan@...ek.ca>,
        Jordan Crouse <jordan@...micpenguin.net>,
        Emma Anholt <emma@...olt.net>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
>  	switch (param) {
> +	case MSM_PARAM_COMM:
> +	case MSM_PARAM_CMDLINE: {
> +		char *str, **paramp;
> +
> +		str = kmalloc(len + 1, GFP_KERNEL);

if (!str)
	return -ENOMEM;

> +		if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> +			kfree(str);
> +			return -EFAULT;
> +		}
> +
> +		/* Ensure string is null terminated: */
> +		str[len] = '\0';
> +
> +		if (param == MSM_PARAM_COMM) {
> +			paramp = &ctx->comm;
> +		} else {
> +			paramp = &ctx->cmdline;
> +		}
> +
> +		kfree(*paramp);
> +		*paramp = str;
> +
> +		return 0;
> +	}
>  	case MSM_PARAM_SYSPROF:
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 4ec62b601adc..68f3f8ade76d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
>  
>  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
>  {
> +	struct msm_file_private *ctx = submit->queue->ctx;
>  	struct task_struct *task;
>  
> +	*comm = kstrdup(ctx->comm, GFP_KERNEL);
> +	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> +
>  	task = get_pid_task(submit->pid, PIDTYPE_PID);
>  	if (!task)
>  		return;
>  
> -	*comm = kstrdup(task->comm, GFP_KERNEL);
> -	*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> +	if (!*comm)
> +		*comm = kstrdup(task->comm, GFP_KERNEL);

What?

If the first allocation failed, then this one is going to fail as well.
Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
string?

> +
> +	if (!*cmd)
> +		*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);

Same.

>  
>  	put_task_struct(task);
>  }

regards,
dan carpenter