From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f181.google.com (mail-pg1-f181.google.com [209.85.215.181]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2AA1A7C099 for ; Fri, 2 Feb 2024 09:26:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.181 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706865972; cv=none; b=Z2fH1jBVFZZNQxpXxONe4l7EbJFGfDH+qt1DexQYGddw9adkvqGoDeOg45Lo6qbtLyoaqlR9U4XfXy+zWnPphoN3A9PGzsemOuqREAlA+AQK4r30/mvcIEXY/l35hM0JJDTMTGoEZQ+wuSwBcVZr2T39079jiewe1xfUP9yPkcU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706865972; c=relaxed/simple; bh=r1KSDdshTOdl1eD90GEdgP1GIyuWH5dotuZGtVETZPw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=d0WReLOkjfRuTzeAerrgLK8ZgK4z80JFZ8uZ3vh/GprO+pkmG4WuJzX1rcHNL565s6HIEjQYfyeaDKq8iAT4Rxg94+zJfLTL1NNzZhgB0dB1PkExT9ZJil5xs4odsciBkY8UFiDDknQ5Lw3tiGtmqC5mUTNh2TC14wUYC2nyKmM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org; spf=pass smtp.mailfrom=chromium.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b=e1wJtdll; arc=none smtp.client-ip=209.85.215.181 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="e1wJtdll" Received: by mail-pg1-f181.google.com with SMTP id 41be03b00d2f7-5d8ddbac4fbso1661441a12.0 for ; Fri, 02 Feb 2024 01:26:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1706865970; x=1707470770; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Lv2NKH9WWB74jvhBNElogS2Uwp+4e3LBP2aAu2jyOBc=; b=e1wJtdllY5htY7mrWKLoyLFT/f7Q4YClbuYH9O/Asnj8KJuVEZK+QCDVoVPlscQQWd 6+2nCpxQYSBRsZHI2VpvJWuugJCA8hZaVEOCCwvmx9Ako7BcR3Ax7+Ae0K3MURBGa+jp 0e92KZB1bBgiiV3kDcC/5a06/Hy32x6Al92cg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706865970; x=1707470770; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Lv2NKH9WWB74jvhBNElogS2Uwp+4e3LBP2aAu2jyOBc=; b=M2w7mzixnJk6zOcZ+TU95ePCtvGoKxwLh3lSTEAMH8ucVuKN2Pk7kFeAtnwu+sSoiE gvpgx1QegS0ARyNiq1CJCxxx6RO8BkgyZr6IittOfpu++1t0MwJcZsfTva0J2/2XuUIB sjv3pP2utPeIzOVyaKUv/6W3fDidCh7ziQ8P9BQARRjXeUBLWnJuJR1vRTK6tsbHyd9U okK5Wpk4uf6L5cj8NlIFkWqesi/CMcPVu3lmKgNFenMPfQwIGqXQzk6fsIX9rLWImHhq pVuSNW7HiTYCvEhPPjPWCNZ7ZFmlv3SevYYOcAufjDv47DY795ICeLHFJGqZ2Qy82DPG uwwQ== X-Gm-Message-State: AOJu0Yxn+WdcSLqxcyQi2W14Owmeckk0e5QKZ8YteBD4nyAHzk9Lg1fm Qg0UJVFoLqy3/kJfEuFwmazeLj6cmW+W5CCKfq8FgDxiDi8QSOW6H6685tyD+A== X-Google-Smtp-Source: AGHT+IFVXwt3ZODto1D6uO78oFjr3U556S4oIO8F6WGJXjqUqMx9HKzgXSKcznjwitA2R8bviSg61Q== X-Received: by 2002:a05:6a21:2c82:b0:19c:9d4d:7d7 with SMTP id ua2-20020a056a212c8200b0019c9d4d07d7mr6590870pzb.41.1706865970497; Fri, 02 Feb 2024 01:26:10 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCUlcw3+jmJncLOKzKlDlKSkf2ZjgJtgJGOC6FlRhNPhCDEe66sr6BxngOa4HC33wqxuh7mE+IJLP3MzYEsY30WoPHLK+jYHPtb15IcNR79Mk8UQnW/N4kc9DVrfxHSxMKktDwocU9ZBml81w4EFV3Wf1zFWnVclBXHDqDogvyS6Qcdt27Yf+IEZ27WTYj2nPUjEbBCVzE2+y6TpV32SvGjcakPZQVKZSxISv7zZTSQVIWXKx8SNFabr9j5ye+/i95QcRGRpWjENb/zMVJBBIY7fCGQA5xisKvcb7THROKTn6PuAj68u0E0bftE3cv9SUnOpuNABN3SB8QAXy6RWhwugPDsqK1OfFKI9r/oKPicqE2qibeM6AafHPmB5XaqwOKzq/hbKaIYBhsoXlM8yScllZ525aboabholHCbQXPxiq5CU/NYCc0g3rXNlxMNWsKHUUKY1zeG/UU1q0mBq/iFMuI3ghFwadThzaI8jzHkVwg+oPA== Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id p20-20020a056a0026d400b006dfedc6a43dsm1140656pfw.208.2024.02.02.01.26.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Feb 2024 01:26:09 -0800 (PST) Date: Fri, 2 Feb 2024 01:26:09 -0800 From: Kees Cook To: Rasmus Villemoes Cc: "Gustavo A. R. Silva" , Andrew Morton , Nathan Chancellor , Nick Desaulniers , Bill Wendling , Justin Stitt , llvm@lists.linux.dev, linux-hardening@vger.kernel.org, Mark Rutland , Miguel Ojeda , Marco Elver , Jakub Kicinski , Przemek Kitszel , Masahiro Yamada , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/5] overflow: Expand check_add_overflow() for pointer addition Message-ID: <202402020105.0759D4CD@keescook> References: <20240130220218.it.154-kees@kernel.org> <20240130220614.1154497-2-keescook@chromium.org> <6d66deda-e09d-4899-b3a3-5137eeee449c@prevas.dk> Precedence: bulk X-Mailing-List: linux-hardening@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6d66deda-e09d-4899-b3a3-5137eeee449c@prevas.dk> On Wed, Jan 31, 2024 at 09:35:35AM +0100, Rasmus Villemoes wrote: > On 30/01/2024 23.06, Kees Cook wrote: > > [...] > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > index 6f1ca49306d2..d27b58fddfaa 100644 > > --- a/include/linux/compiler_types.h > > +++ b/include/linux/compiler_types.h > > @@ -375,6 +375,16 @@ struct ftrace_likely_data { > > /* Are two types/vars the same type (ignoring qualifiers)? */ > > #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) > > > > +/* Is variable addressable? */ > > +#define __is_ptr_or_array(p) (__builtin_classify_type(p) == 5) > > That magic constant is a bit ugly, but I don't think there's a better > way. However, a comment saying "pointer_type_class in gcc/typeclass.h in > gcc source code" or something like that might help. Do we know for sure > that clang uses the same value? I can't find it documented anywhere. Very true. Offlist, Keith Packard suggested I switch to this, so we can avoid the constant: +#define __is_ptr_or_array(p) (__builtin_classify_type(p) == \ __builtin_classify_type(NULL)) > > __check_ptr_add_overflow() - Calculate pointer addition with overflow > checking > > + * @a: pointer addend > > + * @b: numeric addend > > + * @d: pointer to store sum > > + * > > + * Returns 0 on success, 1 on wrap-around. > > + * > > + * Do not use this function directly, use check_add_overflow() instead. > > + * > > + * *@d holds the results of the attempted addition, which may wrap-around. > > + */ > > +#define __check_ptr_add_overflow(a, b, d) \ > > + ({ \ > > + typeof(a) __a = (a); \ > > + typeof(b) __b = (b); \ > > + size_t __bytes; \ > > + bool __overflow; \ > > + \ > > + /* we want to perform the wrap-around, but retain the result */ \ > > + __overflow = __builtin_mul_overflow(sizeof(*(__a)), __b, &__bytes); \ > > + __builtin_add_overflow((unsigned long)(__a), __bytes, (unsigned long *)(d)) || \ > > + __overflow; \ > > + }) > > So I've tried to wrap my head around all these layers of macros, and it > seems ok. However, here I'm a bit worried that there's no type checking > of the destination. In particular, the user could perhaps end up doing > > check_add_overflow(p, x, p) I tried to make sure the top-level filtering would require a pointer to an integral type. I'm sure there is a way to foot-gun it, if one tries hard enough. :) > > which will go horribly wrong. Do we have any infrastructure for testing > "this should fail to compile"? It would be good to have, not just for > this, but in general for checking our sanity checks. > > Another thing is that this will always fail with negative offsets (if b > has signed type and a negative value, the multiplication won't fit in an > unsigned type). I think __bytes should be ptrdiff_t. Ew. A negative "add"... yes. I'll take a closer look. Thanks for the review! As it turns out, I may not need this patch at all yet, so I may hold off on it until I can prove that we really will need it. -Kees -- Kees Cook