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: <8401c328-ed67-8d5e-4ba2-b487f256e139@intel.com>
Date:   Tue, 10 May 2022 09:19:56 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.8.1
Subject: Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in
 fb_release()
Content-Language: en-US
To:     Javier Martinez Canillas <javierm@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        <linux-kernel@vger.kernel.org>
CC:     <linux-fbdev@vger.kernel.org>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Helge Deller <deller@gmx.de>,
        <dri-devel@lists.freedesktop.org>,
        "Daniel Vetter" <daniel.vetter@intel.com>
References: <20220505215947.364694-1-javierm@redhat.com>
 <20220505220413.365977-1-javierm@redhat.com>
 <753d0350-42dc-389b-b10b-4533ddcf32ac@intel.com>
 <1f788b8f-0bea-1818-349e-b1bc907bf251@redhat.com>
 <a339df59-9e00-c7cb-e33d-2ac626443639@intel.com>
 <3b7fe4fe-fdec-cef2-4e0e-309d9dc4a8af@redhat.com>
 <b5ab1c49-04e7-36c3-677d-2989b79e50ca@suse.de>
 <2bf27b09-0896-1849-254f-d5b19abdc892@redhat.com>
 <fc3e8a40-664f-07ae-7474-c0412a1ab1b5@intel.com>
 <1c36d431-d5c0-7278-c9e0-61867e9dc174@redhat.com>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <1c36d431-d5c0-7278-c9e0-61867e9dc174@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eEhIaHozeFlNMmhKRTRGb2FkMzcrVE1UdUp6QndIMDgrUWFHSWp0MExveW5S?=
 =?utf-8?B?bExIZUc0bVVOTmV6VVZOMldzclQ0THhtUG5GMXJ5dXV2UGE4L3pEOEU5amtp?=
 =?utf-8?B?NEpoQlZ5a2UxOUs1dStrb0o2WXhJRlBXN1p5Wk4zZGFHbzl5Vmg2QmM5Y0hG?=
 =?utf-8?B?T0JVeU1qWE5QenVuTU85b3A2OHBBZDhzSWZvVHJ2WEgrNW5zOURObWthd3dO?=
 =?utf-8?B?TFZIMzIzK0JXNy9Gd3AxbEtLUkxteU0xR3FFQ3ZKczZsYktJdW9vSFpGbGJY?=
 =?utf-8?B?Q2FGNVZVaVhQTmlEaFBkaWhiOUlKVXZxODEvbGpxbWk0WVZBRDB2N00rY29a?=
 =?utf-8?B?Qi9GbzhxUHBUOU44YU5pbXkyTVpvcWgxUStPbmx5bDlYQ3FRdXBFdG9qUHh5?=
 =?utf-8?B?aExnMkRsVTJ2NE56RkRkK2phdG5ZQlZqcHllTDM5alFTOUJwazAvMExYUUpP?=
 =?utf-8?B?TkF2QkpMSTc1QXJkTHV2U3Rua2JlcTdPZlF1MU4zUjg5ZENqcEk0eVhlajVJ?=
 =?utf-8?B?cGxPSFl2YThHTFhJTnN3YzhFOTErRitkS3VUME1EZW1NaE5rcDA4YWlueUcx?=
 =?utf-8?B?VzFYTysraDNpdzFLQUxTdVdqa3VVY1VLS1MxcHZTaUU5V0FxZDc2dHpsZDFJ?=
 =?utf-8?B?eERDWit5S0N1ZS9qWUE5dms0bkY1cTFpSFFXS0hiRmhUNU9pbVc4Wmd0aElk?=
 =?utf-8?B?NGUwd0hZZ3FWOG1mY3FsRzg2b1AzOUlobDRrYzJyUjcxVEJDUFl1VzkrQ1RC?=
 =?utf-8?B?UUl5VFVPcVRHMDFJbkQyZVgraDEzQlM2WjF0b2pKS25zSmFYc0Z4WW9qL0Fp?=
 =?utf-8?B?RlZRbSt2cGZCbGxyeGdDbjEwZTVzWnMrNkx6RlZ5Y1RnaklDNE9PVFNyRFhq?=
 =?utf-8?B?Zjk0NnVQeHRrcjBmb0tmTkZ6NDdPRVlXR2VTTkFBS3FSWUVDeE14ZnJCNUJh?=
 =?utf-8?B?bGx2UGZDUWYvVVgxZUE1YUp4bzM3RURON1JuS05DZ2tSRW9ldVozaXZNbU96?=
 =?utf-8?B?SFd5Q1NqR1FqWkVTRXFZVDgzdUd4eUpwZS9SakhTbXJacFdTSDZhbytlUkhH?=
 =?utf-8?B?Y1RCUkE1UFA2em1MSkU0MzRxbVd3RXBNUTlzZjZZUzU2c1Fod0gyeVpyOThK?=
 =?utf-8?B?UFhKaWVqTnY2QzQ2bnhHME54Rld0YUFRM2p2eFQybW9ZdUY1NXVKQ21vZTRm?=
 =?utf-8?B?blUxSHBSTnQyTDBxbzR3TmpycGVVeTJVcWlyU0R5TWlqMDNzekdmTVZqRTIw?=
 =?utf-8?B?dC90alVQeEpqSGZPTGlMdkRDMzFYYjM3QjgwWFcvVWRvUVA0RisrYTMzMVFo?=
 =?utf-8?B?bDgyU3BoZDgwRHRmSjZBL1VDNFE2czkwUnlja1kzY205ZVk3Q1R4SmVrOWVu?=
 =?utf-8?B?YWx6bDBOcktpeldWS3puaHpWMVorZkxQQkNya2M1SVYrY3JxbGl5M3crVE5L?=
 =?utf-8?B?Q1ZiZStqeUNvMUZ2V1gyY29WZDlPRktCS2lDdGxHbVV3OE90QzRIdnNWeWt1?=
 =?utf-8?B?TGRXd0R2Uy9XZTFZYU04TzZIM2pMdTNIV1RjYzYyZnZadjBUNW5ERG1vb2ll?=
 =?utf-8?B?THN6eFBsN2xQbTBaRnYwVU9TOEZud1JvZXpWbjg1U1JGS0JPT0xzRGszV0Zi?=
 =?utf-8?B?TXgyNzVOS1NyaUljL0FYdkNVRUttUFZhQStwN1FjTWlVaEVhc0IyTnJrRklW?=
 =?utf-8?B?YVBYQWN6Nll6RXJDZkgzTW5GdFA4Sk5rNFZDVGtOaWloZXpkZU92N3JFR1hW?=
 =?utf-8?B?TTREQnpUdWFGOGEwT1ZoOTZFTzduR05jSXc0VXRNeDQva1duMkdGbHFuTFg5?=
 =?utf-8?B?eFdpVlNyMHAxd1dCaW5GTmhxbElsdlV1cGxBaU1tU0RodGlIMkFPYTVJdGp0?=
 =?utf-8?B?Tm5yVEFQMW1ZMGJhNmpXcTRMdHFDcEZYZW13SzRET25wK1Rab1Yzb1JQdnUw?=
 =?utf-8?B?RVgxSEpkeTBwSTRMZXo1REwwcm4wbGNFVElvd2NKd09XY1pRb0xKNmVvbno2?=
 =?utf-8?B?aHNac2J1QXlCRjRPOUx1VW1zOVk2ZDNINnpyMjBmZ2Y0eDZ0VEdHdXVUaW1N?=
 =?utf-8?B?UEgrenNEWFo3N1k4dkZ0cTlPdEJqZkVyWTZyemJVanowcW5Od3crY2JDa2w5?=
 =?utf-8?B?dlBxSVdURG52a0VSL0RlQ1h1RWpWY1c2Sm13cXY3eXRZTXgxaThmNWd3OU9Y?=
 =?utf-8?B?czFOR2FmclVIMTVLWis4UHRlOVVBUFhUR1pCZE1HOTE2NGh3TEhoMmMzRTg2?=
 =?utf-8?B?L1llU1AzTVpGamRQYVppY2ZWMDlyUE4zM3BHanQ1RWtGQlBkejlTUWU3bEVP?=
 =?utf-8?B?blVFdE45NzVseWRxZVBlK1NFS1hBTkFwQ3RmQjRPeFUrQWd2Nkg3TThoYlpT?=
 =?utf-8?Q?F49DQZ+6YDFSB3vE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a059ad8d-8d9a-4a38-90e3-08da32558002
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2022 07:20:03.0263
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YRUXWRo74ykg5Y91rHXWHHbilROf+/V+XDEoZ3+3EbcqWSiY9j0S/4bvG/pr+cej21WBUuGxja0uXAuMfoWMkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5144
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE,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 10.05.2022 00:42, Javier Martinez Canillas wrote:
> On 5/10/22 00:22, Andrzej Hajda wrote:
>
> [snip]
>
>>>    static void drm_fbdev_fb_destroy(struct fb_info *info)
>>>    {
>>> +       if (info->cmap.len)
>>> +               fb_dealloc_cmap(&info->cmap);
>>> +
>>>           drm_fbdev_release(info->par);
>>> +       framebuffer_release(info);
>> I would put drm_fbdev_release at the beginning - it cancels workers
>> which could expect cmap to be still valid.
>>
> Indeed, you are correct again. [0] is the final version of the patch I've
> but don't have an i915 test machine to give it a try. I'll test tomorrow
> on my test systems to verify that it doesn't cause any regressions since
> with other DRM drivers.
>
> I think that besides this patch, drivers shouldn't need to call to the
> drm_fb_helper_fini() function directly. Since that would be called during
> drm_fbdev_fb_destroy() anyways.
>
> We should probably remove that call in all drivers and make this helper
> function static and just private to drm_fb_helper functions.
>
> Or am I missing something here ?

This is question for experts :)
I do not know what are user API/ABI expectations regarding removal of 
fbdev driver, I wonder if they are documented somewhere :)
Apparently we have some process of 'zombification'  here - we need to 
remove the driver without waiting for userspace closing framebuffer(???) 
(to unbind ops-es and remove references to driver related things), but 
we need to leave some structures to fool userspace, 'info' seems to be 
one of them.
So I guess there should be something called on driver's _remove path, 
and sth on destroy path.

Regards
Andrzej

>
> [0]:
>  From 5170cafcf2936da8f1c53231e3baa7d7a2b16c61 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Tue May 10 00:39:55 2022 +0200
> Subject: [RFT PATCH] drm/fb-helper: Don't deallocate fb colormap and free fb info
>   too early
>
> Currently these are done in drm_fb_helper_fini() but this helper is called
> by drivers in their .remove callback, which could lead to a use-after-free
> if a process has opened the emulated fbdev node while a driver is removed.
>
> For example, in i915 driver the call chain during remove is the following:
>
> struct pci_driver i915_pci_driver = {
> ...
>          .remove = i915_pci_remove,
> ...
> };
>
> i915_pci_remove
>    i915_driver_remove
>      intel_modeset_driver_remove_noirq
>        intel_fbdev_fini
>          intel_fbdev_destroy
>            drm_fb_helper_fini
>              framebuffer_release
>
> Later the process will close the fbdev node file descriptor leading to the
> mentioned use-after-free bug in drm_fbdev_fb_destroy(), due the following:
>
> drm_fbdev_fb_destroy
>    drm_fbdev_release(info->par); <-- info was already freed on .remove
>
> To prevent that, let's move the framebuffer_release() call to the end of
> the drm_fbdev_fb_destroy() function.
>
> Also, the call to fb_dealloc_cmap() in drm_fb_helper_fini() is too early
> and is more correct to do it in drm_fbdev_fb_destroy() as well. After a
> call to drm_fbdev_release() has been made.
>
> Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d265a73313c9..7288fbd26bcc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>   	cancel_work_sync(&fb_helper->resume_work);
>   	cancel_work_sync(&fb_helper->damage_work);
>   
> -	info = fb_helper->fbdev;
> -	if (info) {
> -		if (info->cmap.len)
> -			fb_dealloc_cmap(&info->cmap);
> -		framebuffer_release(info);
> -	}
>   	fb_helper->fbdev = NULL;
>   
>   	mutex_lock(&kernel_fb_helper_lock);
> @@ -2112,6 +2106,9 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>   {
>   	drm_fbdev_release(info->par);
> +	if (info->cmap.len)
> +		fb_dealloc_cmap(&info->cmap);
> +	framebuffer_release(info);
>   }
>   
>   static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)