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: <9c9d4c86-d77c-adc0-5dc0-d1821a6a3723@intel.com>
Date:   Tue, 23 Aug 2022 11:30:54 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v7 1/8] overflow: Move and add few utility macros into
 overflow
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>
CC:     Andrzej Hajda <andrzej.hajda@intel.com>,
        Andi Shyti <andi.shyti@linux.intel.com>,
        <thomas.hellstrom@linux.intel.com>, <jani.nikula@intel.com>,
        <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <chris@chris-wilson.co.uk>,
        <airlied@linux.ie>, <linux-hardening@vger.kernel.org>,
        <matthew.auld@intel.com>, <mchehab@kernel.org>,
        <nirmoy.das@intel.com>,
        Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
References: <20220816093525.184940-1-gwan-gyeong.mun@intel.com>
 <20220816093525.184940-2-gwan-gyeong.mun@intel.com>
 <Yv10sQADwdZrIV42@alfio.lan> <202208171657.63AE7AC@keescook>
 <f3370fb1-5318-c662-294b-2c7fe693efd6@intel.com>
 <52c09fde-f788-4c2b-efdc-d1783dbc0f6c@intel.com>
 <202208221301.366A33DACA@keescook>
From:   Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
In-Reply-To: <202208221301.366A33DACA@keescook>
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?MitOSGpTaUVZSHpJSU5uN293SzJvUjJWQ0hLT05zN205Z2lWZjRhUnZCWndK?=
 =?utf-8?B?cGJBaGR6S21wdXFDem91US9DWk9oa3cyMnR0ZEsyRWpMTTA2QytmYS96dS93?=
 =?utf-8?B?YlNDRkFlYldFVlB5dlMyZ0xEbTJJZXhKeTIxS25EUmxud0cvcDl3VTRHczh0?=
 =?utf-8?B?L2ZUcCtsMXZhdlhLZWJOZW9CSS8weDJkTlMzWWxXdXpyWG5zQkNoclVSUWVx?=
 =?utf-8?B?OWlaKzNxeFlWS0h1VWJoZzY5b3hxWCt0MmJIOTJOdHBTZjNOczlKM0RzVHd2?=
 =?utf-8?B?VDBycGJEb2ZUTmZGNUVXODlIa01SN1BkTjdTUVBJMVV3RElCWktzTDQwbDNL?=
 =?utf-8?B?WGdZRWQ5M2o4YUJjWkRjSzhKOUN6ZTFIeW9SalVaUDEzbkhTa2NBVnpvcVpp?=
 =?utf-8?B?N1d5QnZEc1R6WGxldVk0djB1b0FPVDFnWDl3K2JwdWZWRkM0VitjN2QwZEdF?=
 =?utf-8?B?byttaUVtQURPRzJ3VWVlbTlzMms1enNNZnJKdCs2cUMwOUZzaHZBWFFUVm1I?=
 =?utf-8?B?STU5ckI1ZjdHdG85RlpNTy8wdmhGYzlLcnhLSm44UGRuQk92U0pPT2Vjbzh5?=
 =?utf-8?B?SC8wejk5NHVXMUlMclFPRTlrREVpd2U0L3pqR1Q3MkU4b0hkdEQ4WW96eWlu?=
 =?utf-8?B?c01DNWRLbnFRYjIzbEl6R3ZBcngxbFZWY214OTRWZHJnRktlODNxekgwdEZV?=
 =?utf-8?B?TWNIejdQRkROUVcwb2U0VXRySjhnTWoybitrUWNaS1RIZnBrMThBaHRtRC8z?=
 =?utf-8?B?cFdER3Q5NGVTSzNqV3ZWdEMrMC9YdkkvTGxXeExUTmgwOTE4VTJIRjhxVzRN?=
 =?utf-8?B?dmV2eXEyaXI5MmVxTkFoZ1BDSTE1QmVGcVVMOHNralpDUEQ0MEZ3RFFPMmxS?=
 =?utf-8?B?Z0VXYkl6M2tHeUxTOWQxZDNOOS9XVFI2MVJ3MGl6ZDN0a3VkZWtNRzdJb0ht?=
 =?utf-8?B?dU5NejUvU1BMYWlpMFd5OHhWbnFDQ01ETUJNZWlVdXNIeThUaDFWaDk3NjQv?=
 =?utf-8?B?MDNOTEhiN2pEZzR6UFlVSzhDR2VjUFVWWG5jNzNLdVRCM2hhckVMRENKTjd1?=
 =?utf-8?B?NzZkMHpsWnkzZjBxQUQ1TEFvenVKdnhPTTlYNXh6NmthdDI2R3pyRkpaSzZD?=
 =?utf-8?B?ZTBzY2ZtOTRwZHpHbkM2SFI4QjY2ZThrYjhKOFhmbGxpRTZaejNVUEFpeUpE?=
 =?utf-8?B?Q3pmMmErczY5N0NYR0NnLytxY2hONUcvSVcvbkZVSXNRdlNianJlVVRtOW52?=
 =?utf-8?B?blkvZ25YN3BaZFlRZTRWbUtuSW5rUjZXdWJOMHRLOVplTHVmQVhDQ3d1Nks4?=
 =?utf-8?B?ckV1NmVRWktTdVVkdkZjQm1pTlNWd0V2YmNBUm9SdXVCd1JRNjBmSlVWSVN4?=
 =?utf-8?B?SlBBM3ZCUG03MTU0V2NoVnJzTk8wQmhIWFpzTXAxUjlhR3RBVWVrWmpNZnNC?=
 =?utf-8?B?cWE3ZVJLL2pjTFNJWEkwZ2tmZHh5bi9FSzBTVVRZbWJ3TWpHSkV1blZiL1dI?=
 =?utf-8?B?b3dSSytqS3UyQjY4dXc5LzRCTmRFTXlBMmtIQzdBcVpYRXg0T3lsOFFYMndB?=
 =?utf-8?B?VGNITWprRUlHUGlSbGNObFEvaWxDek1UYkNKUVlmYXJnb1d1bEs2UjVyb3hs?=
 =?utf-8?B?TlBXNm15RE45OHltdG5ZSmRDVElYTkplVTFESjFUazNkZ3c4YmRockdBTWFD?=
 =?utf-8?B?SUZuV1p3c3NLZFhsenc3S05ndlAzRFJTWDRiRUtrZjR2T1cxdWVJTGViUGlM?=
 =?utf-8?B?TDZXQzMxS1FKVnZYWU8raEpNUFZMb25ySzZoQ0VUYUJDN0dMUlQ4aEFOc3o5?=
 =?utf-8?B?R21HeU5sMENBR3VRTzkzSWVEOEt3a1gwOHM2cU8xb25xRjgwUU1BYWQ3VUlo?=
 =?utf-8?B?WkJWOTZXTUowVWtyWGRDZjRKVndhTmRaZ1R6VFRFcUVjSHRpWmtnMm91TkZj?=
 =?utf-8?B?V2lvQlh5Ukl1UVBLTDJ2bWhsNnJETGlFN2ljaGFXRzdUQzZmTlQyWXlPRWg2?=
 =?utf-8?B?c2k5SndMQVA1Q1lHZmxQMkppeHdkcGJxb3pMOXNJdUw2ZTVSOWk1cWowV0l3?=
 =?utf-8?B?OERIdXcrZFZTZ3NjLzkrS3NVa21tb0ZkaWRobmU0SlNvREkvbi9RUGp0L3F1?=
 =?utf-8?B?bzNzYmhTUHdDb0F1WUdMYzRVSWkwR3prNkR2YjBQdWNLTlFDQ1ZGU2liYmJH?=
 =?utf-8?B?a0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e503878-d83c-4f7e-363d-08da84af89bb
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB5409.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Aug 2022 02:31:09.4674
 (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: XpW48n6Qva8vdys66lB9iS6A6pU7Tt1R4+TKUWvgEHsKHebr3r0mWxigappW3WyMiFUsfGanGYIGzryVXFtSWiEGPMLQRPHxy7HruNNElmc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB2944
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE 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 8/23/22 5:12 AM, Kees Cook wrote:
> On Tue, Aug 23, 2022 at 04:32:10AM +0900, Gwan-gyeong Mun wrote:
>> On 8/22/22 11:05 PM, Andrzej Hajda wrote:
>>> On 18.08.2022 02:12, Kees Cook wrote:
>>>> On Thu, Aug 18, 2022 at 01:07:29AM +0200, Andi Shyti wrote:
>>>>> [...]
>>>>>> +#define safe_conversion(ptr, value) ({ \
>>>>>> +    typeof(value) __v = (value); \
>>>>>> +    typeof(ptr) __ptr = (ptr); \
>>>>>> +    overflows_type(__v, *__ptr) ? 0 : ((*__ptr =
>>>>>> (typeof(*__ptr))__v), 1); \
>>>>>> +})
>>>>
>>>> I try to avoid "safe" as an adjective for interface names, since it
>>>> doesn't really answer "safe from what?" This looks more like "assign, but
>>>> zero when out of bounds". And it can be built from existing macros here:
>>>>
>>>>      if (check_add_overflow(0, value, ptr))
>>>>          *ptr = 0;
>>>>
>>>> I actually want to push back on this a bit, because there can still be
>>>> logic bugs built around this kind of primitive. Shouldn't out-of-bounds
>>>> assignments be seen as a direct failure? I would think this would be
>>>> sufficient:
>>>>
>>>> #define check_assign(value, ptr)    check_add_overflow(0, value, ptr)
>>>>
>>>> And callers would do:
>>>>
>>>>      if (check_assign(value, &var))
>>>>          return -EINVAL;
>>>>
>> Yes, I also like check_assign() you suggested more than safe_conversion.
>> As shown below, it would be more readable to return true when assign
>> succeeds and false when it fails. What do you think?
> 
> No, this inverts the style of all the other check_*() functions, so it
> should remain "non-zero is failure".
> 
Hi Kees,
Yes, I will not invert this part as you commented.
>> /**
>>   * check_assign - perform a type conversion (cast) of an source value into
>>   * a new variable, checking that the destination is large enough to hold the
>>   * source value.
>>   *
>>   * @value: Source value
>>   * @ptr: Destination pointer address, If the pointer type is not used, a
>> warning message is output during build.
>>   *
>>   * Returns:
>>   * If the value would overflow the destination, it returns false. If not
>> return true.
>>   */
>> #define check_assign(value, ptr) __must_check_overflow(({	\
>> 	typecheck_pointer(ptr); 		\
>> 	!__builtin_add_overflow(0, value, ptr);	\
>> }))
> 
> Please don't use the __builtin*s, instead stick to the check_* family,
> as they correctly wrap the builtins and perform type checking, etc. As
> mentioned, check_assign() should just be:
> 
> #define check_assign(value, ptr)    check_add_overflow(0, value, ptr)
> 
> I don't think any of the other code is needed? What's the use-case for
> the other stuff? i.e. Why does anything need overflows_type()?
> 
And, the reason for using the __builtin_add_overflow() built-in function 
directly instead of using the check_add_overflow() function is ,

#define check_add_overflow(a, b, d) __must_check_overflow(({	\
	typeof(a) __a = (a);			\
	typeof(b) __b = (b);			\
	typeof(d) __d = (d);			\
	(void) (&__a == &__b);			\
	(void) (&__a == __d);			\
	__builtin_add_overflow(__a, __b, __d);	\
}))

In this part of the implementation of check_add_overflow()
	(void) (&__a == &__b);
	(void) (&__a == __d);


When comparing the pointer types of a, b, and d, if the pointer types of 
source and ptr in check_assign() are different, a warning may occur when 
building, I used the __builtin_add_overflow() built-in function directly.

Br,

G.G.
> -Kees
>