Received-SPF: pass (google.com: domain of linux-kernel+bounces-34641-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 22 Jan 2024 21:29:21 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org, Geert Uytterhoeven <geert@linux-m68k.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Hugh Dickins <hughd@google.com>, linux-m68k@lists.linux-m68k.org,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Bill Wendling <morbo@google.com>,
        Justin Stitt <justinstitt@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 27/82] m68k: Refactor intentional wrap-around calculation
Message-ID: <20240123022921.77eddqhiltrpphdv@revolver>
References: <20240122235208.work.748-kees@kernel.org>
 <20240123002814.1396804-27-keescook@chromium.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123002814.1396804-27-keescook@chromium.org>
User-Agent: NeoMutt/20220429
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?rfrRRV6FfGhp3+DH6GfIl0nlMQdhO8poHj4yfbcXoVP7Gf4beX7LNP4IB1qw?=
 =?us-ascii?Q?YvLJB94he4trGkfgqbW27SAdlX6NxFv2/Y0KnS99WVWGOcUqV2+F5biXJDok?=
 =?us-ascii?Q?6euQEhcLaBi+Todw3rrF83OAaq9O27X2KADmQIhmowCgCFk0RAC2HgTEsPhH?=
 =?us-ascii?Q?+CIzL1zbiwNY4u+hLZ4Hb89PboZS8CFHzW9WMbkwRbPrnTWJpYI+i5twwEIe?=
 =?us-ascii?Q?ROl321lvNq8zoxH+k7Zo85NodiRRwgArAuJ23q6qcVCwR58LxYlme/WH0naB?=
 =?us-ascii?Q?cNz8v55nyDT865RuWd7oOu9tjUsIGslM9JaqGecKsjbo+qxt8t42rLhfabrO?=
 =?us-ascii?Q?MWii/DUMAyJ67HxfKmpQBPiW78NFsj4UscZQ83uqoJZTJQ0sAgIo5fdDCDeI?=
 =?us-ascii?Q?bMmQkIikC4kqY6sgb2zBhHgMteAHH/Y1IGe6wa0NbjcgJGhRFNlDl7OlNZkR?=
 =?us-ascii?Q?jYCga12jTpipB9zRZAhBwMkdLNGbM0hQEO0ocLXOqMVLG4/QDF/j2q2/gWF2?=
 =?us-ascii?Q?StErr3eZhNVJb5Kr8wyFj4CbhDQsmvEjCEhxend4mJkiLkV74TnPPfHHNXTC?=
 =?us-ascii?Q?TsjW4M8hccUBy+C/eGJuWQuyYMedU/GsLxyP5zYEDU+YtGpE/iEXEUJc1Ncr?=
 =?us-ascii?Q?DsyIKXMBpAp/L2A1vPR7axksWcY2Ce+qRwZUDFZCAQiKvc70wpAG7CkdEwRn?=
 =?us-ascii?Q?oVjyPqEPtnPIuSRhwrtd8hm3gW+6FhGJT+FoPR+vjapRUWrnLF7KwDPovbDg?=
 =?us-ascii?Q?wo2wbLe5GWTrFxdbVeYPOQP1lgSSpZneRgh00yZSMbAFca3RGtekYcH8/wI4?=
 =?us-ascii?Q?jq9K8jn8lPxqZo5DtDMJxHamOYlRRIgOOM+NGWkt4VkJbgbg/lteIms9AyjD?=
 =?us-ascii?Q?n6jNG32WRUYQ6pFAAsJiMlnlkUyGbauGYTlKxtKB1F6wgqkLhEUbwmnw+F6R?=
 =?us-ascii?Q?yr/bxJvuy2FO8jCRZfs3rxKpvNBthteW5cIFPa4clEfKLPpOGp/Hno9X4BEj?=
 =?us-ascii?Q?iZjMtQ55mT+NR9bG33+50bgxVCFD6aL0ojqmFpQBB9Xf2oixG+0ZKShBcNl9?=
 =?us-ascii?Q?TguFrzwUkfZu5pWQQq8V07sQqfeXbCtfCh6KihE9dVI9eQ7/UmAe0BCwupDj?=
 =?us-ascii?Q?9qXvafCUiqUwds6bFVsSDdKGBB0sJlQ7OkD2Cj9LNNuD+g/aubkHcqb0B5+A?=
 =?us-ascii?Q?jvfe9pcETii69l206UP+ISSVf6GOlbyaRVObsua/9sX+FGtZVcUEeVbXkqMf?=
 =?us-ascii?Q?22ZjgZj4vG5wFm8OUxPaU2CpRx70jWX+Am/DNPg87AWUsP0fhgHhhI2+tINf?=
 =?us-ascii?Q?SCQFXUlhaB8wJ1O1Lp7z8qivhFpTljGPWjHiGEBkiEQJTOP+27HaIuusL5kv?=
 =?us-ascii?Q?ZPFwTDE8ISxF42CzlBn6wgJGsHC0uGCFxAr7o7AAYfzcH9eUL7qC30KPVNnB?=
 =?us-ascii?Q?V0SbNJueRTu+P0Bd+zv427amNokj4/rXESgXIsVQSeyYbba8cUX2xKa6tFpq?=
 =?us-ascii?Q?YEnGVodJa0ILxt/NzhzuMf+YyTs1rcI6qFOGje5lHlrs9dVGg2p0I0TrEKBM?=
 =?us-ascii?Q?hQ54akU5wlQem1is9T+u142eirMRZggAFrmkfyWO?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	LU0p7E+7TAC0tDpXpI9cyv+0F4VPxdBzNyK6HzT2BamjobSp8NoWVffTME/S7/C3nTVmPa/YWW2KOrRaxUHVer+fBw8bD38hINsWFi3j3MoQS0SZlG08Ren8O8dDz6YdcMpIZ+cuizxR1YllhYLRK3qlHM952XEB+J6MhzGV2iePBlvE8cgASaehxgJgXEveauXykM5ey3FNtR9aoioWFDbxt1+H1nLeioYwTN0Bf+IWqSsvJuStrFsVLRI6fiuawwShXtv7yBuBS1SUklw3lYV2/2fxCFZXwLvRODI0WffJqOifAFBl61Y3NA3CSTKqN9db0fKoAwSLVU2ELAl8YvT6wgsSDbR9DDJqc91yt/8YtC5AwDVy+shKIQ3hkrIg9HGZph2BzH0Eop2/f/FSVMTfQTFY+K8pSVW5aM3BidJtBh2LB0QeG/5KRirVpD0VqCgpKK9UXdPbKQk7ADfuNpIgu4SjaIpjJSFb8NGxQXar6bqdtaDFHhatIg9/I9j0MG/X945GqvjI1ZS1mvknyQLasrEVtgeS8T8ol9wq3QH0TK1qBiV6nSOD1v430DAEpmlnVncdaKX9Y1T5g+Mr+vVH6fdstt7keoKs8VG5nGo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a380067e-48ea-405a-188b-08dc1bbb1ce2
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2024 02:29:23.8404
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: V4AKez4scciYZ7ghOVcX2VPNz3qkpWtJGSF44xpJMY42KnsAr/Oi7cnmiEV9NEXxBYFDlrfT8TwWgfS65ayv6w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4724
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-22_12,2024-01-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 phishscore=0 adultscore=0 spamscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401230016
X-Proofpoint-GUID: hpPZy6-0mUj1mKyhjCQuSKItHibNI_Wp
X-Proofpoint-ORIG-GUID: hpPZy6-0mUj1mKyhjCQuSKItHibNI_Wp

* Kees Cook <keescook@chromium.org> [240122 19:36]:
> In an effort to separate intentional arithmetic wrap-around from
> unexpected wrap-around, we need to refactor places that depend on this
> kind of math. One of the most common code patterns of this is:
> 
> 	VAR + value < VAR
> 
> Notably, this is considered "undefined behavior" for signed and pointer
> types, which the kernel works around by using the -fno-strict-overflow
> option in the build[1] (which used to just be -fwrapv). Regardless, we
> want to get the kernel source to the position where we can meaningfully
> instrument arithmetic wrap-around conditions and catch them when they
> are unexpected, regardless of whether they are signed[2], unsigned[3],
> or pointer[4] types.
> 
> Refactor open-coded unsigned wrap-around addition test to use
> check_add_overflow(), retaining the result for later usage (which removes
> the redundant open-coded addition). This paves the way to enabling the
> unsigned wrap-around sanitizer[2] in the future.
> 
> Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
> Link: https://github.com/KSPP/linux/issues/26 [2]
> Link: https://github.com/KSPP/linux/issues/27 [3]
> Link: https://github.com/KSPP/linux/issues/344 [4]
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liam Howlett <liam.howlett@oracle.com>
> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: linux-m68k@lists.linux-m68k.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/m68k/kernel/sys_m68k.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/m68k/kernel/sys_m68k.c b/arch/m68k/kernel/sys_m68k.c
> index 1af5e6082467..b2b9248f2566 100644
> --- a/arch/m68k/kernel/sys_m68k.c
> +++ b/arch/m68k/kernel/sys_m68k.c
> @@ -391,10 +391,11 @@ sys_cacheflush (unsigned long addr, int scope, int cache, unsigned long len)
>  
>  		mmap_read_lock(current->mm);
>  	} else {
> +		unsigned long sum;
>  		struct vm_area_struct *vma;
>  
>  		/* Check for overflow.  */

With your nice self-documenting code, you can probably drop that
comment.

With or without the change,

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> -		if (addr + len < addr)
> +		if (check_add_overflow(addr, len, &sum))
>  			goto out;
>  
>  		/*
> @@ -403,7 +404,7 @@ sys_cacheflush (unsigned long addr, int scope, int cache, unsigned long len)
>  		 */
>  		mmap_read_lock(current->mm);
>  		vma = vma_lookup(current->mm, addr);
> -		if (!vma || addr + len > vma->vm_end)
> +		if (!vma || sum > vma->vm_end)
>  			goto out_unlock;
>  	}
>  
> -- 
> 2.34.1
>