Received-SPF: pass (google.com: domain of linux-kernel+bounces-127458-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <cc7defae-60c1-4cc8-aee5-475d4460e574@vivo.com>
Date: Tue, 2 Apr 2024 14:49:07 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue
To: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
 Sumit Semwal <sumit.semwal@linaro.org>, linux-media@vger.kernel.org,
 dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
 linux-kernel@vger.kernel.org
Cc: opensource.kernel@vivo.com
References: <20240327022903.776-1-justinjiang@vivo.com>
 <5cf29162-a29d-4af7-b68e-aac5c862d20e@amd.com>
From: zhiguojiang <justinjiang@vivo.com>
In-Reply-To: <5cf29162-a29d-4af7-b68e-aac5c862d20e@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NkZkL0ZidnNtaHNiR1BMMzFLYTUwa3BtZnNKRnB2eUU4OEtZdjRSVDFqMktI?=
 =?utf-8?B?dUtGRWw1eGpRZHZIS0w2UGZLMXlQa3VncXV2RDJleG9Ob09xSjBHeUtBQldN?=
 =?utf-8?B?WFZCNnE5dWxaenA1S3MvT3dkOVRTSTQ3WmJUSFU5VmZRNkJkZ081aUV1dGd4?=
 =?utf-8?B?OStKbkdzZkYzWTJoSVhqcE96WWZKc3dodHJPbmhCYjdWa1ZYdmRpMlQyUU5q?=
 =?utf-8?B?YysrYUdQZGdCL0xleXNlT0xkUkJhRHVzQWN6WkZmSksxc2RNOVU3YkhSVUMw?=
 =?utf-8?B?WjExcHJCeHgwNnFzQm9BRm15Z2c1V3FpVmJ5ZWhYWU1XMklUUmdJQWZ4YUM0?=
 =?utf-8?B?a1JrL212cm9iaU9sYmRodEVpNGw2TVhTUzRPVmFnc2dqN3NQamtURHVSeHdW?=
 =?utf-8?B?YmFtVTN6bGIyNDdDNmpmazN2WWYrV3pGS0tHT2pjc0dHRjdMdUxubDVXd3hs?=
 =?utf-8?B?MUZPQ05HZE16MVFiYXdzNEJXcnljUVQ2N1E1ck1PU1FULzE4NmprMnh5cHNq?=
 =?utf-8?B?cVgwOVREWVNvZ2YxUlA3SXh4aHVwaHpxekdCbWlzbmZPOC9GVXBpRFVSMCtJ?=
 =?utf-8?B?TzZBQXRRQUFGRnRXNnppRithTllaQmxVRDVTQW5NdkhGMXlrUldpSmdjbUgz?=
 =?utf-8?B?ZXE4ODM1NTBTQy82ckZ2MTJvSDBFcHJRNWRTQkYyc0NXVFp6Nyt4U1UrbXpt?=
 =?utf-8?B?WUd1MnZmVWNNdWJwVlQ2RFVVTm4rR1BLeGV1SHlra0k4c0Z5S1JDN29PcVlj?=
 =?utf-8?B?Z1NHZ1JkREZhcjVqNkNrbXZ0YXcvZjFjNDFMTHJKTm5JYmhmOE5tYlhBd1hl?=
 =?utf-8?B?dXpSQWRqT1Q3WVBYS0p4aUNYaWtodGFxWkw1TGNPc3dkK29hMW5ybXRJUHZn?=
 =?utf-8?B?ZTZjbDU5WkpMTnNLT0Yya3MwSkZvd2lCSHJTUGQ0Z0htcERuaGl2Zis0LzZu?=
 =?utf-8?B?SC90MzVOSkhJRzhmWFprTTlDWmxhVUN0NDcwbG9xYXkweGhIUXdFYkRFMEN2?=
 =?utf-8?B?UUVVSHBVci9uc0YxMUlMbjhScEpNVWorU001Z3FXdGI0eW52ZmN1YjVVQktm?=
 =?utf-8?B?WlhBTFlYejByR1JjQUpqeXhOWCszeDBUeHVRNlQvVU44OUJtZjVESVpHaS9h?=
 =?utf-8?B?eUNEVTlsZFNYS0VtVUUvcisyc1lzeFZ1dEo5NUFYcHJlL3hQSHdBZldFdUdV?=
 =?utf-8?B?V0N1SkZyM01mYXdmdlBMR2o2RlB4SVg2N0ErZUFJelR4YmRJbGM4RkcwUHBl?=
 =?utf-8?B?MHk1d1R4NVhhbUI5NTJqY3hLaXZFTjRiU0Z3NHVYbzZrMHVtS1RpOCsrVnJH?=
 =?utf-8?B?VFQycmVPa1llSGtjaGpPVVA0ZTlzeE45ODFHNzVmWjE3OHlpdUJHc3JYSHBR?=
 =?utf-8?B?TEk5VFFhK2JuTjU5SjdvaUU4cTY1RnNuWUFOTFZmUnNMOHpHSEFHZERXRnNK?=
 =?utf-8?B?VGZiRkh6dnFHbDU3VFpzRXpqT2RrOFdXWUZETXMvVVoxeldpL0pmRHpsWW55?=
 =?utf-8?B?STN4b3R1Z3crZEFPREo0UWJtZVoyYXE5OTdiTjBFZk5tdEZDcFFneXZIMEpq?=
 =?utf-8?B?c2VFZWphY1VmYjEzK1BCTVZlc0ViM3Yya0xlYThlWjEyZlV3U2NiYytFNTFM?=
 =?utf-8?B?M01jT3p1QzJBWmk4bU83QjVtdy9zdU0vWWJreXJqQ1VCYkx6Z09idDVFQ0pr?=
 =?utf-8?B?Z3ZOSEVuUmFuWVJZUDVOR3dnRUZRSG9PVk8zZlJNM01KOVFGb0MxSnBtTjF3?=
 =?utf-8?B?MktjMXRFS0I2L3dZa3JmWklPeExEZVpXTk9TOExmM3loYnltVEUvRWxCMS85?=
 =?utf-8?B?VHpIMHRCTHo5cHZaL3JSR0RRbUI3RkRxdDQra3JhTlBrY21LeVJEYnlNVmNX?=
 =?utf-8?B?U0lSUGFraGl5K1JsTC8wUFV3ZmhCNUxOVzBES25nUDFxdHJhc202N1YxRWpI?=
 =?utf-8?B?ejI1Z3hhNHJ1OSsvdkYrMTR0YVVIVGF6dFZES1ZaaVRsRmhqMW0wYlpoZjFR?=
 =?utf-8?B?eVU3anlzMnFYaU1JVHFBbzM0OXVOWk14b2pvZ0dTUSszNWFtejdlYUJZYzFa?=
 =?utf-8?B?SVNBcW5zK0c0UDdMdytEVnIyQy84bXBXcGl5SStEY1VnMHliT3VHTTlzYmhx?=
 =?utf-8?Q?dWMrJytSTlMDnjuTAkDbWCAM5?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 86f4a8cb-e1e6-485b-d98f-08dc52e1006b
X-MS-Exchange-CrossTenant-AuthSource: JH0PR06MB6849.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 06:49:10.9089
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pNma3lpOpF+W4MoNXaVzP+mnbulXBvCnOVAlY/Ouv0azA1bHG2UyKVChPxC/BvSSgD3vdzsPp9naLc81bTWJYQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEZPR06MB5365

> As far as I can see that's not because of the DMA-buf code, but 
> because you are somehow using this interface incorrectly.
>
> When dma_buf_poll() is called it is mandatory for the caller to hold a 
> reference to the file descriptor on which the poll operation is executed.
>
> So adding code like "if (!file_count(file))" in the beginning of 
> dma_buf_poll() is certainly broken.
>
> My best guess is that you have some unbalanced 
> dma_buf_get()/dma_buf_put() somewhere instead.
>
>
Hi Christian,

The kernel dma_buf_poll() code shound not cause system crashes due to 
the user mode usage logical issues ?

Thanks


在 2024/4/1 20:22, Christian König 写道:
> Am 27.03.24 um 03:29 schrieb Zhiguo Jiang:
>> The issue is a UAF issue of dmabuf file fd. Throght debugging, we found
>> that the dmabuf file fd is added to the epoll event listener list, and
>> when it is released, it is not removed from the epoll list, which leads
>> to the UAF(Use-After-Free) issue.
>
> As far as I can see that's not because of the DMA-buf code, but 
> because you are somehow using this interface incorrectly.
>
> When dma_buf_poll() is called it is mandatory for the caller to hold a 
> reference to the file descriptor on which the poll operation is executed.
>
> So adding code like "if (!file_count(file))" in the beginning of 
> dma_buf_poll() is certainly broken.
>
> My best guess is that you have some unbalanced 
> dma_buf_get()/dma_buf_put() somewhere instead.
>
> Regards,
> Christian.
>
>>
>> The UAF issue can be solved by checking dmabuf file->f_count value and
>> skipping the poll operation for the closed dmabuf file in the
>> dma_buf_poll(). We have tested this solved patch multiple times and
>> have not reproduced the uaf issue.
>>
>> crash dump:
>> list_del corruption, ffffff8a6f143a90->next is LIST_POISON1
>> (dead000000000100)
>> ------------[ cut here ]------------
>> kernel BUG at lib/list_debug.c:55!
>> Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>> pc : __list_del_entry_valid+0x98/0xd4
>> lr : __list_del_entry_valid+0x98/0xd4
>> sp : ffffffc01d413d00
>> x29: ffffffc01d413d00 x28: 00000000000000c0 x27: 0000000000000020
>> x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000080007
>> x23: ffffff8b22e5dcc0 x22: ffffff88a6be12d0 x21: ffffff8b22e572b0
>> x20: ffffff80254ed0a0 x19: ffffff8a6f143a00 x18: ffffffda5efed3c0
>> x17: 6165642820314e4f x16: 53494f505f545349 x15: 4c20736920747865
>> x14: 6e3e2d3039613334 x13: 2930303130303030 x12: 0000000000000018
>> x11: ffffff8b6c188000 x10: 00000000ffffffff x9 : 6c8413a194897b00
>> x8 : 6c8413a194897b00 x7 : 74707572726f6320 x6 : 6c65645f7473696c
>> x5 : ffffff8b6c3b2a3e x4 : ffffff8b6c3b2a40 x3 : ffff103000001005
>> x2 : 0000000000000001 x1 : 00000000000000c0 x0 : 000000000000004e
>> Call trace:
>>   __list_del_entry_valid+0x98/0xd4
>>   dma_buf_file_release+0x48/0x90
>>   __fput+0xf4/0x280
>>   ____fput+0x10/0x20
>>   task_work_run+0xcc/0xf4
>>   do_notify_resume+0x2a0/0x33c
>>   el0_svc+0x5c/0xa4
>>   el0t_64_sync_handler+0x68/0xb4
>>   el0t_64_sync+0x1a0/0x1a4
>> Code: d0006fe0 912c5000 f2fbd5a2 94231a01 (d4210000)
>> ---[ end trace 0000000000000000 ]---
>> Kernel panic - not syncing: Oops - BUG: Fatal exception
>> SMP: stopping secondary CPUs
>>
>> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
>> ---
>>   drivers/dma-buf/dma-buf.c | 28 ++++++++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>   mode change 100644 => 100755 drivers/dma-buf/dma-buf.c
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 8fe5aa67b167..e469dd9288cc
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -240,6 +240,10 @@ static __poll_t dma_buf_poll(struct file *file, 
>> poll_table *poll)
>>       struct dma_resv *resv;
>>       __poll_t events;
>>   +    /* Check if the file exists */
>> +    if (!file_count(file))
>> +        return EPOLLERR;
>> +
>>       dmabuf = file->private_data;
>>       if (!dmabuf || !dmabuf->resv)
>>           return EPOLLERR;
>> @@ -266,8 +270,15 @@ static __poll_t dma_buf_poll(struct file *file, 
>> poll_table *poll)
>>           spin_unlock_irq(&dmabuf->poll.lock);
>>             if (events & EPOLLOUT) {
>> -            /* Paired with fput in dma_buf_poll_cb */
>> -            get_file(dmabuf->file);
>> +            /*
>> +             * Paired with fput in dma_buf_poll_cb,
>> +             * Skip poll for the closed file.
>> +             */
>> +            if (!get_file_rcu(&dmabuf->file)) {
>> +                events &= ~EPOLLOUT;
>> +                dcb->active = 0;
>> +                goto clear_out_event;
>> +            }
>>                 if (!dma_buf_poll_add_cb(resv, true, dcb))
>>                   /* No callback queued, wake up any other waiters */
>> @@ -277,6 +288,7 @@ static __poll_t dma_buf_poll(struct file *file, 
>> poll_table *poll)
>>           }
>>       }
>>   +clear_out_event:
>>       if (events & EPOLLIN) {
>>           struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in;
>>   @@ -289,8 +301,15 @@ static __poll_t dma_buf_poll(struct file 
>> *file, poll_table *poll)
>>           spin_unlock_irq(&dmabuf->poll.lock);
>>             if (events & EPOLLIN) {
>> -            /* Paired with fput in dma_buf_poll_cb */
>> -            get_file(dmabuf->file);
>> +            /*
>> +             * Paired with fput in dma_buf_poll_cb,
>> +             * Skip poll for the closed file.
>> +             */
>> +            if (!get_file_rcu(&dmabuf->file)) {
>> +                events &= ~EPOLLIN;
>> +                dcb->active = 0;
>> +                goto clear_in_event;
>> +            }
>>                 if (!dma_buf_poll_add_cb(resv, false, dcb))
>>                   /* No callback queued, wake up any other waiters */
>> @@ -300,6 +319,7 @@ static __poll_t dma_buf_poll(struct file *file, 
>> poll_table *poll)
>>           }
>>       }
>>   +clear_in_event:
>>       dma_resv_unlock(resv);
>>       return events;
>>   }
>