[bug report] drm/amdkfd: CRIU checkpoint and restore queue mqds

[Yat Sin, David]

Hi Dan,
Thank you for catching this. I will look into it and post a patch.

Regards,
David

-----Original Message-----
From: Dan Carpenter <dan.carpenter at oracle.com> 
Sent: Friday, February 18, 2022 2:30 AM
To: Yat Sin, David <David.YatSin at amd.com>
Cc: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: [bug report] drm/amdkfd: CRIU checkpoint and restore queue mqds

Hello David Yat Sin,

The patch 42c6c48214b7: "drm/amdkfd: CRIU checkpoint and restore queue mqds" from Jan 25, 2021, leads to the following Smatch static checker warning:

	drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_mqd_manager_v9.c:344 restore_mqd()
	error: 'ctl_stack_size' from user is not capped properly

drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
   762        int kfd_criu_restore_queue(struct kfd_process *p,
   763                                   uint8_t __user *user_priv_ptr,
   764                                   uint64_t *priv_data_offset,
   765                                   uint64_t max_priv_data_size)
   766        {
   767                uint8_t *mqd, *ctl_stack, *q_extra_data = NULL;
   768                struct kfd_criu_queue_priv_data *q_data;
   769                struct kfd_process_device *pdd;
   770                uint64_t q_extra_data_size;
   771                struct queue_properties qp;
   772                unsigned int queue_id;
   773                int ret = 0;
   774
   775                if (*priv_data_offset + sizeof(*q_data) > max_priv_data_size)
   776                        return -EINVAL;
   777
   778                q_data = kmalloc(sizeof(*q_data), GFP_KERNEL);
   779                if (!q_data)
   780                        return -ENOMEM;
   781
   782                ret = copy_from_user(q_data, user_priv_ptr + *priv_data_offset, sizeof(*q_data));
   783                if (ret) {
   784                        ret = -EFAULT;
   785                        goto exit;
   786                }
   787
   788                *priv_data_offset += sizeof(*q_data);
   789                q_extra_data_size = q_data->ctl_stack_size + q_data->mqd_size;
                                          ^^^^^^^^^^^^^^^^^^^^^^ ctl_stack_size comes from the user a couple lines earlier.  It's a u32 and so is q_data->mqd_size.  This addition can have an integer overflow.

   790
   791                if (*priv_data_offset + q_extra_data_size > max_priv_data_size) {'
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Which means that this limit check doesn't work.

   792                        ret = -EINVAL;
   793                        goto exit;
   794                }
   795
   796                q_extra_data = kmalloc(q_extra_data_size, GFP_KERNEL);
   797                if (!q_extra_data) {
   798                        ret = -ENOMEM;
   799                        goto exit;
   800                }
   801
   802                ret = copy_from_user(q_extra_data, user_priv_ptr + *priv_data_offset, q_extra_data_size);
   803                if (ret) {
   804                        ret = -EFAULT;
   805                        goto exit;
   806                }


regards,
dan carpenter