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;
Date:   Fri, 9 Sep 2022 15:33:01 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Hyeonggon Yoo <42.hyeyoo@gmail.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        "Dmitry Vyukov" <dvyukov@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>
Subject: Re: [PATCH v5 4/4] mm/slub: extend redzone check to extra allocated
 kmalloc space than requested
Message-ID: <YxrsLc23lWFZ5H4X@feng-clx>
References: <20220907071023.3838692-1-feng.tang@intel.com>
 <20220907071023.3838692-5-feng.tang@intel.com>
 <Yxrcmk6hSvHBCGNo@hyeyoo>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yxrcmk6hSvHBCGNo@hyeyoo>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?HRcJmdsSypecUjZ7vTfZVoBxJx6CfFvOXhEA09dw2d2Sa+R8J3eEAIxybxjH?=
 =?us-ascii?Q?wofEiD9Tczeqj7gVpdsiS1Ro3mtzSzpJWe2IEnJdzqUeEqajVaJxe0EqezLd?=
 =?us-ascii?Q?Naw0Wo/lsNq5rNB+qtiAjpid2JIE4E3MTe181YpqyWpoNt6D8+p4pM/er/Wo?=
 =?us-ascii?Q?nJSW0YY4eKpp6Qab/+Q8rH+sIFVI9F/n2C3RJQHz7TW735lgPsLEgNEiDmw7?=
 =?us-ascii?Q?P9qb9YAKCvtYOgt/G/1pLhJfHUWEZObm+183jhuCTYdqzw1MTc8lOoiWhhZB?=
 =?us-ascii?Q?kRY6/pkBETaoUH8CL3Sf8K7RvmtslWMCUbHQEyxAi7ASaxdiUXz7ZJIJV0qI?=
 =?us-ascii?Q?fdihBQKYK5pl0U5t+4bgQzWPsvOE0WD1mNAE+XeEKE+djWJUTbYQkUcDNshz?=
 =?us-ascii?Q?ccFpXsM0yjIfpKkj9lyI9eW3HQTZAKkyskDhsyqBTy6WaNz1K7felScF7MJb?=
 =?us-ascii?Q?re3TGb3pFzLv6HvOAtlQ1K7h/zXkjYBLH6ozVGxQcFUHW1O5lHbYxcj9ETT/?=
 =?us-ascii?Q?0P32Gh8nEaGZqS4ekurzYXa/N/dp5On3TZYWEvrF0CYkYdChO2fgUdkyQ/PU?=
 =?us-ascii?Q?u6HwUv3dBbAucRqoG8OCDteJDIRfept8plTFOPb+z+nQKVcI3pE5/eyk8RS4?=
 =?us-ascii?Q?BxVcpO6gSl+/juLe18beNcTaI+U+tTTqlLsaq7HD+d7HPWGx70C2TCnIhK71?=
 =?us-ascii?Q?VjvryoEaq3e3lD6XE8QKwIetLK85SPCWUTJKvXaFS/IzESB6uJIWwjje1ihi?=
 =?us-ascii?Q?WXg+BduwiBa6KUVIwdO61UBLNgiIEn7Nip4AD1ioz1vnWDdplS05w5FAsqVp?=
 =?us-ascii?Q?pWO3f4KmOTTwM9663ZJiuAVqMMKsBCxgdv988VivHBTqZq6O8fnFaDFOrfuH?=
 =?us-ascii?Q?hlC+Xh8UTe9sT5pZvcItrLQ60OeZyKLwcoX91lskU2jgAeYS0EHE3BMTaYS/?=
 =?us-ascii?Q?PAvPkZa5t5emrUnSZwhN3FH5fbSb7oN4G4CvjrYNN3JJS6oFqn+TdkxPSVN+?=
 =?us-ascii?Q?dFtwV070Jg+JbeHNLzN8rWlBdfo8gglYseY/hanmTR3GydHdXFPJ8btuw/dh?=
 =?us-ascii?Q?WfGs1fvkrMrBu7tWgAgNEUWm/8fYH5egLuhxg+UhA5dBsglnRRM++VH5xbVl?=
 =?us-ascii?Q?sl3tjF/7umrSxChxWO2qgZTyCovUdDd3WnrxdEcz8tDp+Nh33eqt+lMbiCIr?=
 =?us-ascii?Q?I/GOAduOYIEhP7nsAHhDob0EebSjfzIaXMC8PAQ03qIhSWKWaFVVDiytK752?=
 =?us-ascii?Q?8u9Of8F5xjJZ/vk5buZEjyuyjnkteOQ5jf705jazHOsvJbfAz75Avclu0lUo?=
 =?us-ascii?Q?sc+9Aryw9tC7pWJ2ozD1xmJXjz+iXIhn1aqoSMablymMJddnN+r37iSc57Mt?=
 =?us-ascii?Q?uksqog1Px9wzaCtm4DdsqhKO///+XZP3fzUdnrLKh3Bk6faiD+9m0Y21qidZ?=
 =?us-ascii?Q?mShByn5XV6tPU4wMWsMdV632ZFCVxroTDm6X5nHeMprFEuZpxjPQhkGYlwf+?=
 =?us-ascii?Q?OD6GjPTLDKQdpl9snsCI4G0rwZZ6SVqtoUYTf27v7D14jNMmwQfKK4WGUfGE?=
 =?us-ascii?Q?HR+r4B6DfYc6PBkTDCaMskKUzpCxWf+lVjKt/TfG?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f142a764-bcc6-4615-3905-08da92359986
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2022 07:33:33.6585
 (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: YZvo9RNimjVnDUv7qJMgnZcYce7rDLM6jcMjPLJXqHWOkWcgU4xwQRpOH26oQ3BbYzZRXf8LnqUGJi4Fhx+Eaw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6231
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,RCVD_IN_DNSWL_MED,
        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 Fri, Sep 09, 2022 at 02:26:34PM +0800, Hyeonggon Yoo wrote:
> On Wed, Sep 07, 2022 at 03:10:23PM +0800, Feng Tang wrote:
> > kmalloc will round up the request size to a fixed size (mostly power
> > of 2), so there could be a extra space than what is requested, whose
> > size is the actual buffer size minus original request size.
> > 
> > To better detect out of bound access or abuse of this space, add
> > redzone sanity check for it.
> > 
> > And in current kernel, some kmalloc user already knows the existence
> > of the space and utilizes it after calling 'ksize()' to know the real
> > size of the allocated buffer. So we skip the sanity check for objects
> > which have been called with ksize(), as treating them as legitimate
> > users.
> > 
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
[...]

> > -	if (s->flags & SLAB_RED_ZONE)
> > +	if (s->flags & SLAB_RED_ZONE) {
> >  		memset(p - s->red_left_pad, val, s->red_left_pad);
> >  
> > +		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> > +			unsigned int zone_start;
> > +
> > +			orig_size = get_orig_size(s, object);
> > +			zone_start = orig_size;
> > +
> > +			if (!freeptr_outside_object(s))
> > +				zone_start = max_t(unsigned int, orig_size,
> > +						s->offset + sizeof(void *));
> > +
> > +			/*
> > +			 * Redzone the extra allocated space by kmalloc
> > +			 * than requested.
> > +			 */
> > +			if (zone_start < s->object_size)
> > +				memset(p + zone_start, val,
> > +					s->object_size - zone_start);
> > +		}
> > +	}
> > +
> >  	if (s->flags & __OBJECT_POISON) {
> > -		memset(p, POISON_FREE, s->object_size - 1);
> > -		p[s->object_size - 1] = POISON_END;
> > +		memset(p, POISON_FREE, orig_size - 1);
> > +		p[orig_size - 1] = POISON_END;
> >  	}
> >  
> >  	if (s->flags & SLAB_RED_ZONE)
> > @@ -1103,6 +1139,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  {
> >  	u8 *p = object;
> >  	u8 *endobject = object + s->object_size;
> > +	unsigned int orig_size;
> >  
> >  	if (s->flags & SLAB_RED_ZONE) {
> >  		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
> > @@ -1112,6 +1149,20 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  		if (!check_bytes_and_report(s, slab, object, "Right Redzone",
> >  			endobject, val, s->inuse - s->object_size))
> >  			return 0;
> > +
> > +		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> > +			orig_size = get_orig_size(s, object);
> > +
> > +			if (!freeptr_outside_object(s))
> > +				orig_size = max_t(unsigned int, orig_size,
> > +						s->offset + sizeof(void *));
> > +			if (s->object_size > orig_size  &&
> > +				!check_bytes_and_report(s, slab, object,
> > +					"kmalloc Redzone", p + orig_size,
> > +					val, s->object_size - orig_size)) {
> > +				return 0;
> > +			}
> > +		}
> >  	} else {
> >  		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
> >  			check_bytes_and_report(s, slab, p, "Alignment padding",
> > -- 
> > 2.34.1
> > 
> 
> Looks good, but what about putting
> free pointer outside object when slub_debug_orig_size(s)?
 
Sounds good to me. This makes all kmalloc slabs covered by redzone
check. I just gave the code a shot and it just works with my test
case! Thanks!

- Feng


> diff --git a/mm/slub.c b/mm/slub.c
> index 9d1a985c9ede..7e57d9f718d1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -970,22 +970,15 @@ static void init_object(struct kmem_cache *s, void *object, u8 val)
>  		memset(p - s->red_left_pad, val, s->red_left_pad);
>  
>  		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> -			unsigned int zone_start;
> -
>  			orig_size = get_orig_size(s, object);
> -			zone_start = orig_size;
> -
> -			if (!freeptr_outside_object(s))
> -				zone_start = max_t(unsigned int, orig_size,
> -						s->offset + sizeof(void *));
>  
>  			/*
>  			 * Redzone the extra allocated space by kmalloc
>  			 * than requested.
>  			 */
> -			if (zone_start < s->object_size)
> -				memset(p + zone_start, val,
> -					s->object_size - zone_start);
> +			if (orig_size < s->object_size)
> +				memset(p + orig_size, val,
> +				       s->object_size - orig_size);
>  		}
>  	}
>  
> @@ -1153,9 +1146,6 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
>  			orig_size = get_orig_size(s, object);
>  
> -			if (!freeptr_outside_object(s))
> -				orig_size = max_t(unsigned int, orig_size,
> -						s->offset + sizeof(void *));
>  			if (s->object_size > orig_size  &&
>  				!check_bytes_and_report(s, slab, object,
>  					"kmalloc Redzone", p + orig_size,
> @@ -4234,7 +4224,8 @@ static int calculate_sizes(struct kmem_cache *s)
>  	 */
>  	s->inuse = size;
>  
> -	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
> +	if (slub_debug_orig_size(s) ||
> +	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
>  	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
>  	    s->ctor) {
>  		/*
> 
> -- 
> Thanks,
> Hyeonggon
>