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

[Dan Carpenter]

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