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: <81ca92a2-9525-d96c-3739-280f25bd9b7a@intel.com>
Date:   Fri, 7 Oct 2022 19:38:54 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v13 5/9] drm/i915: Check for integer truncation on
 scatterlist creation
Content-Language: en-US
To:     Jani Nikula <jani.nikula@intel.com>,
        <intel-gfx@lists.freedesktop.org>
CC:     <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <mchehab@kernel.org>, <chris@chris-wilson.co.uk>,
        <matthew.auld@intel.com>, <thomas.hellstrom@linux.intel.com>,
        <nirmoy.das@intel.com>, <airlied@redhat.com>, <daniel@ffwll.ch>,
        <andi.shyti@linux.intel.com>, <andrzej.hajda@intel.com>,
        <keescook@chromium.org>, <mauro.chehab@linux.intel.com>,
        <linux@rasmusvillemoes.dk>, <vitor@massaru.org>,
        <dlatypov@google.com>, <ndesaulniers@google.com>,
        <trix@redhat.com>, <llvm@lists.linux.dev>,
        <linux-hardening@vger.kernel.org>, <linux-sparse@vger.kernel.org>,
        <nathan@kernel.org>, <gustavoars@kernel.org>,
        <luc.vanoostenryck@gmail.com>
References: <20220928081300.101516-1-gwan-gyeong.mun@intel.com>
 <20220928081300.101516-6-gwan-gyeong.mun@intel.com>
 <87edvvzxu3.fsf@intel.com>
From:   Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
In-Reply-To: <87edvvzxu3.fsf@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TmRiM2RtRE43U2Rmd2dndGpNNFNTNHFINlQyZUFnbjFTd0t2dk04djlKaVJB?=
 =?utf-8?B?SmMrQTA0dW1VR25LMUdsVnBTdm9JcHl5cTVJRWNybHdQTTNNZ04yd0haYjlO?=
 =?utf-8?B?VlpsRnNtT1IzblJZZUJWVGlaZ3p6UFBZckdRRGdzVWJ6UWJyQUlNckErenp2?=
 =?utf-8?B?UVp6b1d4TzdGaCtIaUJhOUpwa0daRGtVZk5FVTRpaWduZlVPSXZWSTNDWGp0?=
 =?utf-8?B?SG0wUlkyRkNiSndEclJ1UzIzN0ZyamVUazhTOVZId1lGd2pBTVJXOWlNUHQr?=
 =?utf-8?B?SnZDT1FFY09tR0c1SWc0dU4rNlI4U1JqbjNGOW45ZUZ0eHZ5T0VRcnRNMnRQ?=
 =?utf-8?B?enRLaDB6Z2cwR2NhSWlxdlZkU2p6Y1ZWelVVSkV0dVJaNzRDT3RuYTdLM2xX?=
 =?utf-8?B?SHZ6Z2g1TCtadE9qaklCeXM3TzdOT0hEQWdqekRJWHNuZnFpc1JnNEgvbkJT?=
 =?utf-8?B?RlRZRXB0Wk5zV3o4Y1FWeXVPTmQ2ZU4xdzRXMnpsM3l6Q1hXU2xBeVp0c3lI?=
 =?utf-8?B?STQxbzBuRnFLMUNsUXRTdWROalVBUnV1cDc3SkdNNXVGc2N2eEtHN0tnTHo0?=
 =?utf-8?B?RDNKblAwQVhnQ3NQU20rVlNOY3FibkxZSmg2bGMxWXB4L1lvRCsyS0NrSDhQ?=
 =?utf-8?B?RkNYSXQ5aVBsRFVvenhoZmxvTTh2K215OFVaZ01YbnJuOURLYnllSUIzRU1Y?=
 =?utf-8?B?enh0TzNFa1g5eVRkTDljMXBWVC9VdGdTUVFyUjZJU1pLMTZJWWtvbi9tNFlx?=
 =?utf-8?B?VVpYdWNGU1d5OWtBS0xvcmEzdTRlaVhlTno0ejZpOVRqUW0wY29aMTFuYzNK?=
 =?utf-8?B?WjR0dm9EKzFYNWgyV01vTlZ0b0tQSlhPdjVoMUs2TVczUXJGbHZGOWd4Z1dB?=
 =?utf-8?B?TW1VeVQ3SUx1NVRtdVpFd3hrQ0ZVVjhHUWVZSGhSaGVrQklvSExzLzJ5MUxU?=
 =?utf-8?B?dHg0Z01GV25wZU5RWlN4WU9VamRMU0I4N1d6OUR1MGFqK0dzei80OXFCL0hy?=
 =?utf-8?B?eHhMMkoxMmZwZXlUNFNiZ2Y5T2k1SG41c3VZVjd2NE11UUo4ZmxKQndNOG9V?=
 =?utf-8?B?SHQxTEx0YmhGNUJSVkdhK3dkVUUyeFJTUThiYk5yVlB1NWNoTC9tK1BTdmlU?=
 =?utf-8?B?Yy9xYk9GUGZpTUQ3azc3N3dseHNUMXRTNnc2UjRNV1NwM2Z4elQvMDdCUjQ1?=
 =?utf-8?B?S1YxUUI0a0N2SER4Ui8wTzVrYko0OWxjcnE1OGpRRFZJam5DajFpdlJRbjEv?=
 =?utf-8?B?enZudFpLK3REcXlZKzk4RXZZdlEyaUFWZmZrdzdUdkpIQVdsSGxUVWJRbHZ5?=
 =?utf-8?B?UXdET0tpbm5DUVd4dllMUjF0Snd3bEl1S3QrUkJPYzhNd2FPNWZ3KzJUS3RY?=
 =?utf-8?B?WTFqVmt6UkZ0WmlDNUxvcmtVUytvZklyU1gyL2Y4ZWwrSlNaVldOdHpCL2d1?=
 =?utf-8?B?L05Hc2FpYTUwN2RUcXkxZDcwV2pBaXUycmVEYVF2Szc4QkRJbEZjT2hmWndO?=
 =?utf-8?B?R21kL29VTFI1aURNUnpSZjVzakVId0ZGQTA4cloyaEkwdENKNXJPQlJVUWg3?=
 =?utf-8?B?NmI2cDE4OWhEanlMdnFRQXBqOFBycy9GOForUi9pZXQ0MkNzYlc1ZzE0bndO?=
 =?utf-8?B?M2VMQnM0RmF5M09MbVY2eTVSQTk4VWdxRnVxWGRVUE9RczlSS1pJMjIrZzZx?=
 =?utf-8?B?V1N5NEpFbXgwK2dvV25DYnVXSXpPU1hEQ2ZhQ3ZEM0NuKzUvN0FQNHQ5dDZx?=
 =?utf-8?B?R0t1Y1ZDVGUxZHZoV1o2MmhTbnl1dHpaYkJBZ1lSWmYvSW1JVTQyNGNnM1Fs?=
 =?utf-8?B?Sk0rbllXeWxkdmxFZHU2aitZTGlTblN2ci9QeGgrKzVUZCsxRGh3UmNOVWtM?=
 =?utf-8?B?RFl3ZE5oN0FsT3B0dTlUNTlOMXQ1enUrL2V3STAxZHhld3pkeWE0OTlOejBP?=
 =?utf-8?B?V2MyNHZURDl3VFhCRWhFSlFnUGt0NDByN2wxNjdPYldrUnRMZ3RaMEhEdU5h?=
 =?utf-8?B?ODIxUDNId0hTRGh0UUZsUlpsMFJlSXMrc05rNXV3bGVMWmZmZTFGVFFuT01G?=
 =?utf-8?B?L0NBTXk1L0l4dE1HR29uVkNiVlJPMm1pRDdxaDBZTUtaRTRJYUdTd1lORDBP?=
 =?utf-8?B?RXBmZDFzWjlVazhoaU94UDg4WjB3N2xneUZrcW1vcE85UDc4UnB4TktUYXBo?=
 =?utf-8?B?Y1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e73d50da-5bd6-4150-15e1-08daa88288cf
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB5409.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Oct 2022 16:39:42.6246
 (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: 37Ze7WNt0IV87+nk8CTq+JbKiIm83t2IJLUfS1mRfNWapbsff9XSsg+AN656Rejvx1VcARctyzOFCfneUjGK9yA2p3FgJyYchixMeHROmEg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6959
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.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_NONE,SPF_NONE 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 9/28/22 11:51 AM, Jani Nikula wrote:
> On Wed, 28 Sep 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
>> index 9ddb3e743a3e..1d1802beb42b 100644
>> --- a/drivers/gpu/drm/i915/i915_scatterlist.h
>> +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
>> @@ -220,4 +220,15 @@ struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
>>   						     u64 region_start,
>>   						     u32 page_alignment);
>>   
>> +/* Wrap scatterlist.h to sanity check for integer truncation */
>> +typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
>> +#define sg_alloc_table(sgt, nents, gfp) \
>> +	overflows_type(nents, __sg_size_t) ? -E2BIG \
>> +		: ((sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp))
>> +
>> +#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
>> +	overflows_type(npages, __sg_size_t) ? -E2BIG \
>> +		: ((sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, \
>> +						       size, max_segment, gfp))
>> +
>>   #endif
> 
> No. I don't think we should shadow sg_alloc_table() and
> sg_alloc_table_from_pages_segment().
> 
> Either get this in scatterlist.h (preferred) or prefix with i915_ or
> whatever to indicate it's our local thing.
> 
> i915_scatterlist.h already has too much scatterlist "namespace" abuse
> that I'd rather see gone than violated more.
> 
> 
Hi Jani,
Thanks for you comments.

I will update this patch by removing the shadowing of 
sg_alloc_table_from_pages_segment() / sg_alloc_table(), so that the 
caller checks when overflow checking is required.

Br,
G.G.

> BR,
> Jani.
> 
> 
>