Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a9fe9bc4-b6b2-444d-2a6b-500637c96906@amd.com>
Date:   Wed, 24 May 2023 20:27:33 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2] amdgpu: validate drm_amdgpu_gem_va addrs
Content-Language: en-US
To:     Chia-I Wu <olvaffe@gmail.com>, dri-devel@lists.freedesktop.org
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        =?UTF-8?B?TWFyZWsgT2zFocOhaw==?= <marek.olsak@amd.com>,
        Yang Li <yang.lee@linux.alibaba.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Philip Yang <Philip.Yang@amd.com>,
        Luben Tuikov <luben.tuikov@amd.com>,
        Mukul Joshi <mukul.joshi@amd.com>,
        Danijel Slivka <danijel.slivka@amd.com>,
        Jammy Zhou <Jammy.Zhou@amd.com>, amd-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20230523225336.2642008-1-olvaffe@gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20230523225336.2642008-1-olvaffe@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SklHNlJZRnhOc2VCUTU0ZjhxNVhyRHhKQWlJOXFtOFVMTVNCTFdxQ3V6OVhP?=
 =?utf-8?B?SmpOUTFwanVCOGdHemQza1M4MWVMUCtXbnExcE83L3RCRGpGU0llY1JzN2xN?=
 =?utf-8?B?bTZLNjZqczRDRUliU1U3eStuWUVFN3M2VFlWQlJUZFJJcjlpOTdYa3JaNFc2?=
 =?utf-8?B?MXBWVTh5bWVIWXpCdk1oWjE5NGxyem1kNC9vZGUwbXFXTzRUWVJaOHhDU3pM?=
 =?utf-8?B?OHVwTWs5akszaWRNZE9qblRFWEVSMVNjTXpudW1pU2tFT0tZT3FQL3czVGdQ?=
 =?utf-8?B?N01MZWtZdE9jNXlQQWZzNDJLU24xZUhnNkcwNGdxeXFRR0hTd2R2WS9mUnF4?=
 =?utf-8?B?MmE3NWQrdTFmZmd4N2RISmFiVFd0aEdSMWVHUVJEWFRqZ1hiY3hXUnhXWTZw?=
 =?utf-8?B?TURzZjdVLzU1c1ZMTTl3ZlRUaTMyUjJSdDJOdnRiMDNqWU1jUDc0SE53ejUw?=
 =?utf-8?B?cGFCb091N3RuMDYzaXU1UXo2SlVYTS9XT1N1RitUWk5RdUh1WGEvT1IvNnJH?=
 =?utf-8?B?Y2hjM2NqMEplalFqYUg1MnRkYUdjc3NZVTRNQytaMWdZRWJNdWRQb3BGa3o0?=
 =?utf-8?B?enNNTEp1N1VRWkhEQ3RRSlh6LzZWeTIxdGh2VjhYdnA1V2NCc0x5WHRmeDNt?=
 =?utf-8?B?Rnhic3dBaUU3MTJYMXBVYTIzZUNkY3hjVW01aFExTW9Qb04yQ2FXRElqOWF0?=
 =?utf-8?B?M2FUS1J3ODA4dEZCQlpaRG5OYzhaZkh0cGVnN2FrRm81NUlmMW9RcG1jRDhP?=
 =?utf-8?B?bUxDOWt4UDFnRE1aNjk5OVFVdUFhZWZQN1R4TTJuTWFIODk2c1F2VW5zeGVP?=
 =?utf-8?B?QW56S3o4NjVtbmh5NG9POFZ6Yk1DR3pkaERpM28zNS9GQlkwcUtoMjA4R2ZH?=
 =?utf-8?B?K3ppWmkyQzNEanVobkQ3M3BVaVhTa0lQUHRkV3QwZ2g1aU9mRWtXTzIyUUNS?=
 =?utf-8?B?RVZnbTZoWk9KUjJWWUhnN3g1SGVRdGJYVVBtQ2JHa2o5UGp5ZVlSV0Z5ZHZV?=
 =?utf-8?B?MFIxN2Z3Z2VyQXl2NGpQRlZOTjY4dHNXMHB6cjdxVElNOHVJZThnTER3V3Vj?=
 =?utf-8?B?UmYxWC9tREtVQWtuN2RFT0NoVkkyTjFhMmFOYnhoM0hoTU5xQmF2clJ5bDFp?=
 =?utf-8?B?dlZsYjFoU1FLcjh2L2hVSDdYMFh1M21VY1F1MXdqWXJzbzRjWVhPMjErU3Rt?=
 =?utf-8?B?a2tFTVZSSVZCTU5CS2dLaG42ZEkvQ3h1R01leHVMbmhpRlFYeERRR2pqdWRG?=
 =?utf-8?B?TGlCbVZFTnVSVk1Ga0FweXIzTzBRV1o1VTh1T1prbC9BaFd0clhzK05rY2RH?=
 =?utf-8?B?NXBzUDUrMzhKa0NFWE1YaW5KanVhbER2MXBHVHFndi8yLzdjOE5hNi9aWC9N?=
 =?utf-8?B?citORkpCZlc0Zjczalg2d3pNd3NMVHZzdG5NTUZKN1ZpckRTMm5MNVRWWUtY?=
 =?utf-8?B?WnRleTZSVnUyUWtDZmdId05DUTNmaU5MZXpnTnRKUkZld2RrZ1NmMGI5bk0v?=
 =?utf-8?B?UmRMaDZqNUF2ejZ0R3gxb0hVQUI0UkNER1RvR3RxM25kNFdGejRqVHFHdG5p?=
 =?utf-8?B?djJQUjhDVG5GTUorNlEvWEZnbkF2eWMwOFlBbENwZHg1akpEZSs1RDNkS0R3?=
 =?utf-8?B?UE5DVVRQYXJrRXppNEtjYWpJOExtRndESlBRbDZhUnpYazNLdElQSS9jUXRI?=
 =?utf-8?B?VmQwcjg2UHpzbjRkKzdENXNSRG1ybVNiL3FHWFZpamtwamRIc0l0ZmpPRDhX?=
 =?utf-8?B?YVE0a1dnVXlsaDBWRkY4QytyS3Z1K1Jhb1QzY04vT1JqUFJLdzNPYzM1UStR?=
 =?utf-8?B?dkVOclp3Y1RmeWp3elVSSjNvQjAxT3J5S1dPcEJvNHoyQ2RLR2xHcWw3czQy?=
 =?utf-8?B?MUV4bUk0WkM3ZWVocXU4eWNhTllJSjd4OFlhaXlzUDl6YW9zUzQ2VmtWNUda?=
 =?utf-8?B?bGV4Z2ZuMTQzbDVQdG9uTGVrd0pJVUVVWk5wQUJsaVBubzdwYkNPa0xTL2dF?=
 =?utf-8?B?aHpCTGhSUzlCU0VzcU12bU52eHRQeGlRR1F5d1R3RGVER1hybjhXRE8yTHVH?=
 =?utf-8?B?dHcxWVg2MjVmU0pFTU9NMWhPOE5NdENGRFM5QXdrb3JJaXZISDg5Vkc4UEll?=
 =?utf-8?B?bi8yREx2VGpIdExOc1JCUjlqbHNTa3BpbTZWTDF6ZWxmWFpPMXAvcTJIamlF?=
 =?utf-8?Q?irdnB+acPS9crMonQKbpuccwrPHYLCJzrp3f2Sae4OTR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1cccff9f-1cc8-4df9-704b-08db5c848eea
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2023 18:27:41.0361
 (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: vjMJss8r8mXm9Omh94Gs/nDeD+M+pGpZ2w9DLF7EFdEnyRRak+DuoFVvgd9hHj8+
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4879
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 24.05.23 um 00:53 schrieb Chia-I Wu:
> Validate drm_amdgpu_gem_va addrs in amdgpu_gem_va_ioctl.
> amdgpu_vm_bo_replace_map no longer needs to validate (and its
> validations were insufficient either).  amdgpu_vm_bo_map has internal
> users and its validations are kept.

No, please keep all validation inside amdgpu_vm.c code.

See the offset is unused or might have a different meaning for some use 
cases, so validating it here is actually not correct.

Christian.

>
> This is motivated by OOB access in amdgpu_vm_update_range when
> offset_in_bo+map_size overflows.
>
> Userspace (radeonsi and radv) seems fine as well.
>
> v2: keep the validations in amdgpu_vm_bo_map
>
> Fixes: 9f7eb5367d00 ("drm/amdgpu: actually use the VM map parameters")
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 15 +++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  |  8 +-------
>   2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index d8e683688daab..36d5adfdf0f69 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -681,6 +681,21 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   	uint64_t vm_size;
>   	int r = 0;
>   
> +	if (args->va_address & ~PAGE_MASK || args->offset_in_bo & ~PAGE_MASK ||
> +	    args->map_size & ~PAGE_MASK) {
> +		dev_dbg(dev->dev, "unaligned va_address 0x%LX, offset_in_bo 0x%LX, or map_size 0x%LX\n",
> +			args->va_address, args->offset_in_bo, args->map_size);
> +		return -EINVAL;
> +	}
> +
> +	if (args->map_size == 0 ||
> +	    args->va_address + args->map_size < args->va_address ||
> +	    args->offset_in_bo + args->map_size < args->offset_in_bo) {
> +		dev_dbg(dev->dev, "invalid map_size 0x%LX (va_address 0x%LX, offset_in_bo 0x%LX)\n",
> +			args->map_size, args->va_address, args->offset_in_bo);
> +		return -EINVAL;
> +	}
> +
>   	if (args->va_address < AMDGPU_VA_RESERVED_SIZE) {
>   		dev_dbg(dev->dev,
>   			"va_address 0x%LX is in reserved area 0x%LX\n",
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b9441ab457ea7..6307baaa136cf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1501,15 +1501,9 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
>   	uint64_t eaddr;
>   	int r;
>   
> -	/* validate the parameters */
> -	if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK ||
> -	    size == 0 || size & ~PAGE_MASK)
> -		return -EINVAL;
> -
>   	/* make sure object fit at this offset */
>   	eaddr = saddr + size - 1;
> -	if (saddr >= eaddr ||
> -	    (bo && offset + size > amdgpu_bo_size(bo)) ||
> +	if ((bo && offset + size > amdgpu_bo_size(bo)) ||
>   	    (eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
>   		return -EINVAL;
>