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: <2c762b15-fd7e-f14e-fcc9-a083af683e4f@suse.com>
Date:   Thu, 25 Aug 2022 11:50:25 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
Content-Language: en-US
To:     Juergen Gross <jgross@suse.com>
Cc:     Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        stable@vger.kernel.org,
        Rustam Subkhankulov <subkhankulov@ispras.ru>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org
References: <20220825092600.7188-1-jgross@suse.com>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20220825092600.7188-1-jgross@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ekVOb1ArYUxRZFpFa1dYNnQwclBoeDlSSlcxSXNaOWZWV3BQOTAxN2xzZ3Ez?=
 =?utf-8?B?cUgxd2ZUUFB0NnhDRzlzOHd4MllYaUVpZWRmd3ZnRnZCbXFBa29ZbTJuNEhz?=
 =?utf-8?B?RWM1RjVXQ1daMzRVWm1aNEFkeDlsaUFEbjhOYmRRYm1wRDN5TnRrdksxRElF?=
 =?utf-8?B?VTdwV0hiSldNeTFYR3RDRDBxODYrZnV5Yk5HVG80N2NrdFFHZkFOSWdpOTRD?=
 =?utf-8?B?bGR2dkVUeVE5Q3IrV1RQTmVZK2drenNhek1UOEw4eHlCdHZ3RHFVNHRKR0FE?=
 =?utf-8?B?Zy9Va1dmQWZqSTBEVEdkL1ExMGdoeFlQWDBpOVg4S1lBczN5V0REQm92VFhp?=
 =?utf-8?B?cEVaQjFyVXBmem9HYitmRm1vUGtabFI5SmQrS2JYWGpPenkxbkRwZlJHcUtL?=
 =?utf-8?B?NHg0RGk5WUYvRlhiTU1PWnRXL3psdWtEL3VIWDdXaTNyVFN3UXBlOSszRkU1?=
 =?utf-8?B?V2xxUVY2am9MME4vTUoyT0FLd1J6UDVYS21wclhpWmtGWjE4SFprN2xPM1ZH?=
 =?utf-8?B?ODRueXp1TnJlWWVtNlRXVUZGR1JFTlBVYUVoRng3WXM5akVpRTd6aFNraDM0?=
 =?utf-8?B?aFc0cTlvaGNmbUxjOTh0a0k3OXJNWStGV291NzNJUlNHdnBZZWFPb3pzUno1?=
 =?utf-8?B?Q1R0TktCclJPNDRqVTNSUk90UXNxaTJNZXlaT1pmQVhpS2xQK1dOaHdjQU9s?=
 =?utf-8?B?TCtEUlJVcTZEZmpHSWZKMWVxc1JYYTNBdUZkN0dXNGFQbWo3RGhIL3BrQmRu?=
 =?utf-8?B?eVEzWjRyZjlRSmZIbi9OSHhIaVZtYXp5NmpneGcxQm5kRmN5RDM0NFYwTmJT?=
 =?utf-8?B?Nzg5eGJ5UkVuTXpzZmVNbnMxL0J5UlI2M2dCSVI2TnpCTk5ZeW5yZ1g1UVJK?=
 =?utf-8?B?bDh5SFZRdnpaZDYyOThnYVozUFo0a1BibkVFOE1jSENxcjQzMzhrejUrZEgr?=
 =?utf-8?B?OEdjWmRNOTNrWElINngrS2kzZU04dTFkVWg5NDYzWnE2bGE5QmY3dnRqUi9D?=
 =?utf-8?B?RG5BcWJMNnRBK3NBTWR0alg0REdWMVdhSGRHVTVSSTg4bGE3YkNoWEt0UC82?=
 =?utf-8?B?SW5sam55RGVXYmxnNHcvbE1CS3VBNVpqQm93YnZGd3FySVUrdTJJN1RZLzhZ?=
 =?utf-8?B?aXJTUzJNTDJjcHRSNytjWkxwSmUzWExBaVJtRkcyejI1QnFqSEF4YUJwQ0xL?=
 =?utf-8?B?T0pwZUVMOUlGcjRFTnhLWXBEYUVGY0tkV3J6VDZMYnRjMS9QdmNnd0pLUXNx?=
 =?utf-8?B?ZjNXT1hraXV6ZWQySzhZbG9ZSnJoODlKTUpQenMxTlowL0JSL0hJTm1yZWt4?=
 =?utf-8?B?akMxNWhEbXJUMk11WDVTSDdJWXZLbXVxbW9yV096T0pEVjR0R3VxVmZLcWMw?=
 =?utf-8?B?b2UxRXlXVFlEdHFCajduL3BlNklvOStBWDRGZE50UjJxdDAxeUEvNXNPL0FH?=
 =?utf-8?B?RG1NZGlIL3JWZGdkSG82YysxK25Eb2t5Um9aR0prNzh4ZFdZdkRFNFVJNnpM?=
 =?utf-8?B?VHNQamJ1NzdWNDF1RFptcWsydHJUbWEzOGFzelpOWGlmTTAreFN1Umpla1o0?=
 =?utf-8?B?Yk01RURoYmg4N3k0SzhrTER5ZHJXaUxpbmRBWk9wSHBlbUZpUTNSRjlPb3Ir?=
 =?utf-8?B?MGJPQitlNnd0Mk9FQUMxdzJabUNQTTF6dHVpSmRUMkVaNk9BaXloRnNjM3M4?=
 =?utf-8?B?WXRRTG1rS1lPcC9lZndRSndabnpBUGxaNGpYOEdGS3RWUzZvYjZPOUNxZ0xq?=
 =?utf-8?B?Z3VGS2RBN3FTYXEzVU9XdWdMWFowcUJMWE40MDNNNWtqZ1UvR3ZmUVM0ZGZ4?=
 =?utf-8?B?cGk2RCtJSnJHeE9rYjZGbFkzWnAzYS8vcllZVCtKbTd5VzlPOWhyY251T3Iw?=
 =?utf-8?B?N0wzcDF4SDFZWDVKVjhUYVN5d080TVk5SGxRci9MZWc2T1VudzNvZEZUZTU4?=
 =?utf-8?B?cVFRSjZnN1QreVRHcFJ4dVppd09ybnd5ZFFFWmZLem1kWU4yK2dWeHR0a0hn?=
 =?utf-8?B?bHU4OHBhaGY4MTRhQW5ZblM2dUh4V2ZaZi9iUkcybll2SzVoWkhZQitDdnoz?=
 =?utf-8?B?aElzWnkrKzk4L09xWmZEdFl3TmxPYzRoMVJlTkk3OE5zUXZmUDFqME96MFMy?=
 =?utf-8?Q?09A5vrtdjHtnoyIGp8OitZJno?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f97a8ffe-43bb-448f-282c-08da867f3d60
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Aug 2022 09:50:27.7058
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eFseiJNM2EyK04kOXO0WoAMstTmnVQX2gsWzdHdjngHzKgBJslyU5kW8Xq6gUmOapOvZaT1t1WRrO/212h9zEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0402MB2737
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham 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

On 25.08.2022 11:26, Juergen Gross wrote:
> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
> 
> Additionally lock_pages() doesn't check for pin_user_pages_fast()
> having been completely successful, resulting in potentially not
> locking all pages into memory. This could result in sporadic failures
> when using the related memory in user mode.
> 
> Fix all of that by calling unlock_pages() always with the real number
> of pinned pages, which will be zero in case pages being NULL, and by
> checking the number of patches pinned by pin_user_pages_fast()

Nit: s/patches/pages/

> matching the expected number of pages.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I have a question / suggestion, though:

> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -602,6 +602,10 @@ static int lock_pages(
>  		*pinned += page_count;
>  		nr_pages -= page_count;
>  		pages += page_count;
> +
> +		/* Exact reason isn't known, EFAULT is one possibility. */
> +		if (page_count < requested)
> +			return -EFAULT;
>  	}

I don't really know the inner workings of pin_user_pages_fast()
nor what future plans there are with it. To be as independent of
its behavior as possible, how about bailing here only when
page_count actually is zero (i.e. no forward progress)?

Jan