Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <067ee7e8-6dc3-4e84-84fe-bc00e1193848@amd.com>
Date:   Thu, 12 Oct 2023 08:48:54 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in
 dma_fence_enable_sw_signaling()
Content-Language: en-US
To:     "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc:     Sumit Semwal <sumit.semwal@linaro.org>,
        Gustavo Padovan <gustavo@padovan.org>,
        Arvind Yadav <Arvind.Yadav@amd.com>,
        linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
References: <ZSarP0/+hG8/87//@work> <202310110903.FE533CBCD@keescook>
 <0d63e8aa-5836-40e7-8f8a-2be10fbaac4a@embeddedor.com>
From:   =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <0d63e8aa-5836-40e7-8f8a-2be10fbaac4a@embeddedor.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?UnNseFdSMVh4SU9vdng3cVB0eHcxdlVNREpySno2elEvVUloT2Frczk0bU1y?=
 =?utf-8?B?QVNHbUNKYUFCTGQyKzFDMEM4VkNGeEorOW1QK0ZRN0ZsRmtNRUlqd0UybFVY?=
 =?utf-8?B?aFdBV200bEJOZjB3bWg1NEV1eDlwTkwwcGM3QXQ1Mm0xOC9qZWQvTnBBMTgr?=
 =?utf-8?B?TjVjR0lZT1hqSGJDcGhGOVVNRkY4bVZpQTc5YXVydFBpSVpTV1Z6c3dpaEtK?=
 =?utf-8?B?Zit0eEkvdW5rMXJNYmo2aGdVajE5V3ptaUhBaFRXc1NXMk1JaGN0YVgrZGE5?=
 =?utf-8?B?b2h1SW9ncFp2dTQ3WnlNdkZObTlaSGJ6VEpYOVJyRFRhcVY4cjNYblpCMksx?=
 =?utf-8?B?NWliQzBpTjVrMkpxLzIzdy9Yd09FamJxREovK1liRWxYa2dmSGptR2RWRVZl?=
 =?utf-8?B?Z3EzZWdMZFgrdG1ReFhFUDBOOHZycDhLV0RZYVBOSDNWbjF1L3hwVVc4Sk1m?=
 =?utf-8?B?VFh6LzY4RmNyaTg0aTBNWVh0aVplU29yNkU5QzhrMWJnRnVWWnc0bW5OWEtW?=
 =?utf-8?B?MWhPdTU2VGFtMHZLRXNqZGc2TUpEZTZ3TUozRTlmZEdGcXpKUlBFMy8vZm1x?=
 =?utf-8?B?QThwdW1lT3lVZS95L0VhdkdrMFJYNDRPU1QzaFlnbXF1TkhqZk1SNXNtckxL?=
 =?utf-8?B?WGNCelNtUkczQ3dvZmtmSHVidEo5S1JVczBISldlanFBajZMYm1nMldZM1Ja?=
 =?utf-8?B?ZlRIbWF2bWpGWnQ5ZWFXU1RyajJGQWdDVHV0YkNPeVVFVFVndElaaHowaXhy?=
 =?utf-8?B?OS9zN0xDMjFmNlFORlNTMWFCbDhWU0RoZEsxWW4yT3VSK2JmbENNKzJwWnRi?=
 =?utf-8?B?Vmg0NXJRTld1a0ZzNk1nNml5Q1lKdU5mNlNsVVdyOWYrdzE1WkZaUkJlaEI5?=
 =?utf-8?B?ZkxmaWIzWWR5K0hsNDZ5Q1VLa2pDZk5Nc01LdDhmaWdYc2JXNndPRldGSTZK?=
 =?utf-8?B?a1dXSzR5cG1ablRseDhxN0d2TWQ4eGs5N1ltR0xqU1NKSkRicUxmNG9JQ3I1?=
 =?utf-8?B?RWpsa0NJWHhESUJKaWx4eUo1bVRtQ0s4MFpIWnd6dEZXUldpQm5mSGE1ZVc0?=
 =?utf-8?B?MGd6eHRLWkRFMGVjZzI5bCtYdjVRVVJ1NWZPWGN3ZjNyVnZONFZMdXMrREdz?=
 =?utf-8?B?dkFGb1JsWW8xVTBta1Q1MXQvaHJ1RENzYXVBcHhOaVYyejV3N0h1WThFS1J1?=
 =?utf-8?B?Y3M1QVNURHFJU2srQzRwUGtxbTZqU3Y0NThMMlk4Y0Y1Vlkxd05PQWQ4am5i?=
 =?utf-8?B?K21WdkdSK3NVNk12ZXFTL3A4MENuaElJME9YK3U3eEpXQWJTZUQxWUFzL0ZO?=
 =?utf-8?B?MVNURWZMaGI3QXFyUTg2S2ZkWURXQWhIMys2OE1Ob1dDODMwOTJjaGJ1LzZX?=
 =?utf-8?B?MEhFRFlsQW1EeDdLRFRTUHE2QnNidWxlZDA4SytkWC9nc0JUMWZ5dko0eXBH?=
 =?utf-8?B?RlZwU3huM0pxMjhwdWlxaE1TS3lLQUxMd212Skp0dlBhNzBndGw2RXdxM1Zp?=
 =?utf-8?B?VEJuY0t0SUczejBZMXE3cStNL1FRd0QvWTNsQ2pjUVV5eDQvSmkyZklUMWN6?=
 =?utf-8?B?dFhZSnk1bkZkRk1VNHdISm9iZS8zZUM5R0VXRitobE9vS0ZJQ3FaNXFXcW1N?=
 =?utf-8?B?bUVReDd6bHJtS2U3K3N4bkVMaGpjeHhBZlRGYWxCdmhWbnkxZmNNWklZZVZu?=
 =?utf-8?B?Slo1T092dzJTTk9zYWVBc0pKUHBIRm5hanoxWVlPSTRXdGNzVVl5cUpBaXVK?=
 =?utf-8?B?VkY3K3p5WVlaU2pCaGFzb2JBTnNpZ0IvZWU5U0d1TCtEL3Nqb1Y5dldxc1ZI?=
 =?utf-8?B?cldRaGROMjErTTAyejJ0dDJza1E5SlN1QTVhUG9ZS3Frd2UwZXFoQkM3clJx?=
 =?utf-8?B?ckl2S0RQQUxnb2U5UUxOaVQySFczOXJhemZkWmpHQmgwbTliUkJqWnRyU0d0?=
 =?utf-8?B?TWdFRXZ1aTVDbmR6akx6Y2lRV3dOQkdVays4SHJYajRUMHN6aWFxTkR5NnJ0?=
 =?utf-8?B?Vnh2YjB3eVlOU3Z2MkNPTmVOWHRDQlNOcGNQMUNLcFlKa3FaQTVsQnBUaTVX?=
 =?utf-8?B?c3JMUjJJVWJyR0VQbXNPV2FnMExjazk2WTRMODhWVGFXbVhHVGh3L1psdkdw?=
 =?utf-8?Q?qLeU=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd69f58b-3cc5-43a7-09fa-08dbcaef4ff9
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Oct 2023 06:48:59.2053
 (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: o5H1w3RP/Cu4mytWgZq5HBboMKLluJOe0gPdqSfxkt3GO0U0Chk1OIeJr5SZKxA2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB8216
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 11 Oct 2023 23:49:22 -0700 (PDT)


Am 11.10.23 um 18:13 schrieb Gustavo A. R. Silva:
>
>
> On 10/11/23 10:03, Kees Cook wrote:
>> On Wed, Oct 11, 2023 at 08:03:43AM -0600, Gustavo A. R. Silva wrote:
>>> Currently, a NULL pointer dereference will happen in function
>>> `dma_fence_enable_sw_signaling()` (at line 615), in case `chain`
>>> is not allocated in `mock_chain()` and this function returns
>>> `NULL` (at line 86). See below:
>>>
>>> drivers/dma-buf/st-dma-fence-chain.c:
>>>   86         chain = mock_chain(NULL, f, 1);
>>>   87         if (!chain)
>>>   88                 err = -ENOMEM;
>>>   89
>>>   90         dma_fence_enable_sw_signaling(chain);
>>
>> Instead of the larger patch, should line 88 here just do a "return
>> -ENOMEM" instead?
>
> Nope. I would have to add a `goto` to skip 
> `dma_fence_enable_sw_signaling(chain)`.
>
> I originally thought of that, but as other _signaling() functions have
> sanity-checks inside, I decided to go with that solution.
>
> This bug has been there since Sep 2022. So, adding a sanity check 
> inside that
> function should prevent any other issue of this same kind to enter the 
> codebase
> and stay there for years.

I'm trying to remove those sanity checks for years since they are hiding 
problems instead of getting them fixed.

Calling dma_fence_enable_sw_signaling with a NULL pointer is a coding 
error and not a recoverable runtime error.

The test case should be fixed instead.

Regards,
Christian.

>
> -- 
> Gustavo
>
>>
>> -Kees
>>
>>>
>>> drivers/dma-buf/dma-fence.c:
>>>   611 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>>   612 {
>>>   613         unsigned long flags;
>>>   614
>>>   615         spin_lock_irqsave(fence->lock, flags);
>>>                    ^^^^^^^^^^^
>>>                     |
>>>               NULL pointer reference
>>>               if fence == NULL
>>>
>>>   616         __dma_fence_enable_signaling(fence);
>>>   617         spin_unlock_irqrestore(fence->lock, flags);
>>>   618 }
>>>
>>> Fix this by adding a NULL check before dereferencing `fence` in
>>> `dma_fence_enable_sw_signaling()`. This will prevent any other NULL
>>> pointer dereference when the `fence` passed as an argument is `NULL`.
>>>
>>> Addresses-Coverity: ("Dereference after null check")
>>> Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for 
>>> selftests")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>> ---
>>>   drivers/dma-buf/dma-fence.c | 9 ++++++++-
>>>   include/linux/dma-fence.h   | 2 +-
>>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 8aa8f8cb7071..4d2f13560d0f 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -607,14 +607,21 @@ static bool 
>>> __dma_fence_enable_signaling(struct dma_fence *fence)
>>>    * This will request for sw signaling to be enabled, to make the 
>>> fence
>>>    * complete as soon as possible. This calls 
>>> &dma_fence_ops.enable_signaling
>>>    * internally.
>>> + *
>>> + * Returns 0 on success and a negative error value when @fence is 
>>> NULL.
>>>    */
>>> -void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>> +int dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>>   {
>>>       unsigned long flags;
>>>   +    if (!fence)
>>> +        return -EINVAL;
>>> +
>>>       spin_lock_irqsave(fence->lock, flags);
>>>       __dma_fence_enable_signaling(fence);
>>>       spin_unlock_irqrestore(fence->lock, flags);
>>> +
>>> +    return 0;
>>>   }
>>>   EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
>>>   diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>> index ebe78bd3d121..1e4025e925e6 100644
>>> --- a/include/linux/dma-fence.h
>>> +++ b/include/linux/dma-fence.h
>>> @@ -399,7 +399,7 @@ int dma_fence_add_callback(struct dma_fence *fence,
>>>                  dma_fence_func_t func);
>>>   bool dma_fence_remove_callback(struct dma_fence *fence,
>>>                      struct dma_fence_cb *cb);
>>> -void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>> +int dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>>     /**
>>>    * dma_fence_is_signaled_locked - Return an indication if the fence
>>> -- 
>>> 2.34.1
>>>
>>>
>>