From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DDCBAC433FE for ; Thu, 24 Nov 2022 12:05:55 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F8DF89916; Thu, 24 Nov 2022 12:05:54 +0000 (UTC) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by gabe.freedesktop.org (Postfix) with ESMTPS id 46043895EE for ; Thu, 24 Nov 2022 12:05:51 +0000 (UTC) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.54]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4NHxST0x94zJnr8; Thu, 24 Nov 2022 20:02:17 +0800 (CST) Received: from [10.67.110.176] (10.67.110.176) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 24 Nov 2022 20:05:33 +0800 Subject: Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export To: Charan Teja Kalla , =?UTF-8?Q?Christian_K=c3=b6nig?= , "T.J. Mercier" References: <20221117062152.3029018-1-cuigaosheng1@huawei.com> <99d3aee6-ba3e-5333-6f79-ddbcfc0e8843@amd.com> <2c9fa595-e788-5474-4f2b-ffbd08a70d13@amd.com> <83944425-c177-7918-bcde-9cf7296a613f@amd.com> From: cuigaosheng Message-ID: Date: Thu, 24 Nov 2022 20:05:33 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.110.176] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemi500012.china.huawei.com (7.221.188.12) X-CFilter-Loop: Reflected X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Pavan Kondeti , dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, sumit.semwal@linaro.org, Dan Carpenter , linux-media@vger.kernel.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Some tips: Before we call the dma_buf_stats_setup(), we have to finish creating the file, otherwise dma_buf_stats_setup() will return -EINVAL, maybe we need to think about this when making a new patch. Hope these tips are useful, thanks! On 2022/11/24 13:56, Charan Teja Kalla wrote: > Thanks T.J and Christian for the inputs. > > On 11/19/2022 7:00 PM, Christian König wrote: >>>     Yes, exactly that's the idea. >>> >>>     The only alternatives I can see would be to either move allocating >>>     the >>>     file and so completing the dma_buf initialization last again or just >>>     ignore errors from sysfs. >>> >>>     > If we still want to avoid calling dmabuf->ops->release(dmabuf) in >>>     > dma_buf_release like the comment says I guess we could use >>>     sysfs_entry >>>     > and ERR_PTR to flag that, otherwise it looks like we'd need a bit >>>     > somewhere. >>> >>>     No, this should be dropped as far as I can see. The sysfs cleanup >>>     code >>>     looks like it can handle not initialized kobj pointers. >>> >>> >>> Yeah there is also the null check in dma_buf_stats_teardown() that >>> would prevent it from running, but I understood the comment to be >>> referring to the release() dma_buf_ops call into the exporter which >>> comes right after the teardown call. That looks like it's preventing >>> the fput task work calling back into the exporter after the exporter >>> already got an error from dma_buf_export(). Otherwise the exporter >>> sees a release() for a buffer that it doesn't know about / thinks >>> shouldn't exist. So I could imagine an exporter trying to double free: >>> once for the failed dma_buf_export() call, and again when the >>> release() op is called later. >> >> Oh, very good point as well. Yeah, then creating the file should >> probably come last. >> > @Gaosheng: Could you please make these changes or you let me to do? > >> Regards, >> Christian. > . From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C6B8C43219 for ; Thu, 24 Nov 2022 12:07:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229763AbiKXMHV (ORCPT ); Thu, 24 Nov 2022 07:07:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43950 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229558AbiKXMG7 (ORCPT ); Thu, 24 Nov 2022 07:06:59 -0500 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 828DA70A18 for ; Thu, 24 Nov 2022 04:05:50 -0800 (PST) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.54]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4NHxST0x94zJnr8; Thu, 24 Nov 2022 20:02:17 +0800 (CST) Received: from [10.67.110.176] (10.67.110.176) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 24 Nov 2022 20:05:33 +0800 Subject: Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export To: Charan Teja Kalla , =?UTF-8?Q?Christian_K=c3=b6nig?= , "T.J. Mercier" CC: , Dan Carpenter , Pavan Kondeti , , , References: <20221117062152.3029018-1-cuigaosheng1@huawei.com> <99d3aee6-ba3e-5333-6f79-ddbcfc0e8843@amd.com> <2c9fa595-e788-5474-4f2b-ffbd08a70d13@amd.com> <83944425-c177-7918-bcde-9cf7296a613f@amd.com> From: cuigaosheng Message-ID: Date: Thu, 24 Nov 2022 20:05:33 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.110.176] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemi500012.china.huawei.com (7.221.188.12) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Some tips: Before we call the dma_buf_stats_setup(), we have to finish creating the file, otherwise dma_buf_stats_setup() will return -EINVAL, maybe we need to think about this when making a new patch. Hope these tips are useful, thanks! On 2022/11/24 13:56, Charan Teja Kalla wrote: > Thanks T.J and Christian for the inputs. > > On 11/19/2022 7:00 PM, Christian König wrote: >>>     Yes, exactly that's the idea. >>> >>>     The only alternatives I can see would be to either move allocating >>>     the >>>     file and so completing the dma_buf initialization last again or just >>>     ignore errors from sysfs. >>> >>>     > If we still want to avoid calling dmabuf->ops->release(dmabuf) in >>>     > dma_buf_release like the comment says I guess we could use >>>     sysfs_entry >>>     > and ERR_PTR to flag that, otherwise it looks like we'd need a bit >>>     > somewhere. >>> >>>     No, this should be dropped as far as I can see. The sysfs cleanup >>>     code >>>     looks like it can handle not initialized kobj pointers. >>> >>> >>> Yeah there is also the null check in dma_buf_stats_teardown() that >>> would prevent it from running, but I understood the comment to be >>> referring to the release() dma_buf_ops call into the exporter which >>> comes right after the teardown call. That looks like it's preventing >>> the fput task work calling back into the exporter after the exporter >>> already got an error from dma_buf_export(). Otherwise the exporter >>> sees a release() for a buffer that it doesn't know about / thinks >>> shouldn't exist. So I could imagine an exporter trying to double free: >>> once for the failed dma_buf_export() call, and again when the >>> release() op is called later. >> >> Oh, very good point as well. Yeah, then creating the file should >> probably come last. >> > @Gaosheng: Could you please make these changes or you let me to do? > >> Regards, >> Christian. > .