Subject: Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference
 in radeon_driver_open_kms()
To:     Zhou Qingyang <zhou1615@umn.edu>
Cc:     kjlu@umn.edu, kernel test robot <lkp@intel.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20211130155720.178214-1-zhou1615@umn.edu>
 <20211201032214.26889-1-zhou1615@umn.edu>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <d7c890d8-9535-b349-0c28-b08e5074f714@amd.com>
Date:   Wed, 1 Dec 2021 08:20:11 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <20211201032214.26889-1-zhou1615@umn.edu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T2swc1dsTDBQQ3lIRXRYYUFhbTdadjg2SXV0K001aG9WY29BR09LOTNCeWtN?=
 =?utf-8?B?Y2NMcktlNGs4QzhzelRCUGdYTGw2NElWWWFhUWZrYmhNY0NndHNWTklEd2Rw?=
 =?utf-8?B?SEN5NjBUNDFwdzlMRm5kYzErM0JsL1djcVlwWDYzcmhQQ09FR1pPdkdEanVz?=
 =?utf-8?B?ZExReGlsdnlTVHNUOXpSbHFydDRrWXhCUHZKMjJtRWtweEcyKzh5VXdqdDRS?=
 =?utf-8?B?QkRhNzB0V2lqdkZjSGRjdklzUmV4TVdESURCN0lXZTJSV1RhSUYxdjJXWmZF?=
 =?utf-8?B?T2pkWEhNVjhiQVJUN1QrV0FydENKcUw1ZXdESjBKY2JFMGVkUHVPd3I1VE1n?=
 =?utf-8?B?Q01oOExYYms1MkFOdFQ0K2k3OVIxcThDWjU4RC9ZTEZJcDFqQVU5Z2grcjFP?=
 =?utf-8?B?Sk5wUkY3NWJmeGZGazNzV1lPOHdOaDNRR2hSdDlWTm5GYVNna2QxWmlrWWxG?=
 =?utf-8?B?bXlYWWR1eTNhOVl6QzR5bStxR3F1Z1gxOHkyUzRnaU9qcEthTUwwKzlEdjRZ?=
 =?utf-8?B?M2Nib2M3WlorNjNWbDFCWUpReWJEaTZreTlhL1AzZTlhQ3hML3RHTlhTNEdE?=
 =?utf-8?B?ay83WXpvQ1pSbDNVcERlUllURWhYMTZ1VXdHWTR2blo5LzY0bDgwSGxwZmtH?=
 =?utf-8?B?NVhKbFgwaWJkbUQzSjJ0MGJlY1hMclpGNm1kWCtJNVlnS244azhEeGtpS3JK?=
 =?utf-8?B?Z1o5eWxWWmlBNzlJa3RTQjg1Mm5EdGJPMXRnODJLTzAxQlI4Zjc3WURSZmNB?=
 =?utf-8?B?ZitDRzVkZW1MRktwNVkwZmZXRTFJM0pSeEd4WWlxZEtudkhjU0VjRmQ5Y3lE?=
 =?utf-8?B?RnY5RFB5Ny90MVNRcERwMERmQ09KbkZ6TEQvVlVpampKS1JrdXRiUlVUMkpM?=
 =?utf-8?B?VWlBdEJCSzFONTRaNUFQNDVLcWwyVzkyeFN0UVBHYmZiYUdHOUh4MStWZndw?=
 =?utf-8?B?a1EwQmJZaWszakdLNER0R2wvaGZ2ZEhlL3JCcG9QdGROS3R6eEJBendiaS9m?=
 =?utf-8?B?a2ZJZkFaN2huSmxmdUJoeXVHemlma2E5Y0s0TEp1ZDQvTUNJcWhHOVltaTZx?=
 =?utf-8?B?N21zWXUxTndpdTVTTkR0Y1JlU21jaldjNzUvZUZxSTBTVml4SkpUU05ZNnFn?=
 =?utf-8?B?UFp3YmtQcFdVclJUa2wwd2I1OXhFTitMSmtZLzZNVXVvR3puWEhGNm1xUlZt?=
 =?utf-8?B?clBUNG9tSk52T1FSdzBVWnlwbmZQTGNVZlFweWl0c1hjSlIya1JwTFZFZzB0?=
 =?utf-8?B?QkJrSDg5d0JNWDlIT3hUQjg3OGhUck9hdFdtWHJ6RUhBWmdBekR5QVNDcVpr?=
 =?utf-8?B?Nk91MEF2U1dKc2pFQy92UzlrSEc4a0xuaVZJaTgrSERhUlExS29PUTlkUHZ3?=
 =?utf-8?B?UjhidU1sM0JpQUFtRlBNcFVjOXFLZ21MSTg3bHE0N2dyL2RpSGJ6THFyRytr?=
 =?utf-8?B?NXJRYkUrd3RBQjFic2QzWmJjRFhOTW9ETy9rbzNNdXIwVlh2S2xZN1JhQkpp?=
 =?utf-8?B?YTFPWk5NZXZ0cEx1UURtTXZWZUVNbGFtbDNSZk0zYjJMV1luaG0xS3c1Sng5?=
 =?utf-8?B?cEhRYkZrU3NqRkRLSDhNTXh0MEI1SUZiRXlDd1JaL01HWFRiVDdZWXNIMGt0?=
 =?utf-8?B?amMyUUpIY2pWdndaZVBUSDdDcmJDajZiRnNhRUEyYnJnM3ZWVGM1dGtuWmFv?=
 =?utf-8?B?MzhzeWgyYmpwZVpLRkRLWGJ3a0xVNHNYb2F3RTgxQ2k4dnZ5Y2ZLZWRYMmNM?=
 =?utf-8?B?MEpOaTJDOU14NlNrb1h0ZjVlR1dQTVM2ZGV5akpVaUd4SkQ0OCsrYTJjaEFl?=
 =?utf-8?B?S2ZtSmxUbFp6YnAwTHhPL1M4R3crblN4dEdncThNeHA3WkRsUWtzWnBnUjJo?=
 =?utf-8?B?RUR6UkZSODJCeW1IOTNPQWFDVkhEWTJndUtCZ3N0VGh5SmxPYlJTcFMzZ2RC?=
 =?utf-8?B?WnNxOSt3QXN3d2Z2NDFIS3ZsQzg4ZWVweHhGcVZnR3g2UEV5WlR4WVFHM0Jz?=
 =?utf-8?B?dXhzOENQdVgzWnMrRFhqK2hPYXFEZkJzVmVTdENpRjgzRFppVnp4TzJzQ2Iv?=
 =?utf-8?B?a2ZDSDVyWTBJTjFNdWtwbGhnOEZLV3hoNkZUdVI5NEN6ZTVxTndGNHQvemZo?=
 =?utf-8?Q?AbMU=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dfddd46b-2e54-48ea-8311-08d9b49b074b
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1201MB0192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2021 07:20:18.7484
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kgYtTihy+12YUJO/4zz4Lwz3Lgi80qlhJZ82zFayxhbS+IhEpqbEROmnez0TqU3i
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1151
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 01.12.21 um 04:22 schrieb Zhou Qingyang:
> In radeon_driver_open_kms(), radeon_vm_bo_add() is assigned to
> vm->ib_bo_va and passes and used in radeon_vm_bo_set_addr(). In
> radeon_vm_bo_set_addr(), there is a dereference of vm->ib_bo_va,
> which could lead to a NULL pointer dereference on failure of
> radeon_vm_bo_add().
>
> Fix this bug by adding a check of vm->ib_bo_va.
>
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
>
> Builds with CONFIG_DRM_RADEON=m show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: cc9e67e3d700 ("drm/radeon: fix VM IB handling")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> Changes in v2:
>    -  Initialize the variables to silence warning

What warning do you get? Double checking the code that shouldn't be 
necessary and is usually rather frowned upon.

Thanks,
Christian.

>
> Changes in v3:
>    -  Fix the bug that good case will also be freed
>    -  Improve code style
>
> Changes in v2:
>    -  Improve the error handling into goto style
>
>   drivers/gpu/drm/radeon/radeon_kms.c | 37 ++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 482fb0ae6cb5..9d0f840286a1 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -648,7 +648,9 @@ void radeon_driver_lastclose_kms(struct drm_device *dev)
>   int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   {
>   	struct radeon_device *rdev = dev->dev_private;
> -	int r;
> +	struct radeon_fpriv *fpriv = NULL;
> +	struct radeon_vm *vm = NULL;
> +	int r = 0;
>
>   	file_priv->driver_priv = NULL;
>
> @@ -660,8 +662,6 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   
>   	/* new gpu have virtual address space support */
>   	if (rdev->family >= CHIP_CAYMAN) {
> -		struct radeon_fpriv *fpriv;
> -		struct radeon_vm *vm;
>   
>   		fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
>   		if (unlikely(!fpriv)) {
> @@ -672,35 +672,38 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   		if (rdev->accel_working) {
>   			vm = &fpriv->vm;
>   			r = radeon_vm_init(rdev, vm);
> -			if (r) {
> -				kfree(fpriv);
> -				goto out_suspend;
> -			}
> +			if (r)
> +				goto out_fpriv;
>   
>   			r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
> -			if (r) {
> -				radeon_vm_fini(rdev, vm);
> -				kfree(fpriv);
> -				goto out_suspend;
> -			}
> +			if (r)
> +				goto out_vm_fini;
>   
>   			/* map the ib pool buffer read only into
>   			 * virtual address space */
>   			vm->ib_bo_va = radeon_vm_bo_add(rdev, vm,
>   							rdev->ring_tmp_bo.bo);
> +			if (!vm->ib_bo_va) {
> +				r = -ENOMEM;
> +				goto out_vm_fini;
> +			}
> +
>   			r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
>   						  RADEON_VA_IB_OFFSET,
>   						  RADEON_VM_PAGE_READABLE |
>   						  RADEON_VM_PAGE_SNOOPED);
> -			if (r) {
> -				radeon_vm_fini(rdev, vm);
> -				kfree(fpriv);
> -				goto out_suspend;
> -			}
> +			if (r)
> +				goto out_vm_fini;
>   		}
>   		file_priv->driver_priv = fpriv;
>   	}
>   
> +out_vm_fini:
> +	if (r)
> +		radeon_vm_fini(rdev, vm);
> +out_fpriv:
> +	if (r)
> +		kfree(fpriv);
>   out_suspend:
>   	pm_runtime_mark_last_busy(dev->dev);
>   	pm_runtime_put_autosuspend(dev->dev);