From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759556AbcHaKS4 (ORCPT ); Wed, 31 Aug 2016 06:18:56 -0400 Received: from foss.arm.com ([217.140.101.70]:56902 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752745AbcHaKSu (ORCPT ); Wed, 31 Aug 2016 06:18:50 -0400 Date: Wed, 31 Aug 2016 11:08:48 +0100 From: Mark Rutland To: Kees Cook Cc: Julia Lawall , Joe Perches , Fengguang Wu , LKML , "kernel-hardening@lists.openwall.com" , Glenn Wurster Subject: Re: constification and cocci / kernel build test robot ? Message-ID: <20160831100848.GB4783@leverpostej> References: <1472325098.26978.14.camel@perches.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 30, 2016 at 02:50:36PM -0400, Kees Cook wrote: > The structures that should get the greatest level of attention are > those that contain function pointers. The "constify" gcc plugin from > PaX/Grsecurity does this, but it uses a big hammer: it moves all of > them const even if they receive assignment. To handle this, there is > the concept of an open/close method to gain temporary access to the > structure. For example: > > drivers/cdrom/cdrom.c: > > int register_cdrom(...) { > ... > if (!cdo->generic_packet) { > pax_open_kernel(); > const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet; > pax_close_kernel(); > } > > (The "const_cast" here is just a macro to convince gcc it's only to > write to a const value, so really it should maybe be called > "unconst_cast", but whatever...) Just to check, are they actually marked as const, or some pseudo-const like __ro_after_init? I can imagine a number of issues with casting a real const away (e.g. if the compiler decides to cache the value in a register and decides since it is const it need not hazard with a memory clobber). > This allows all of struct cdrom_device_ops to be const, even if they > need to be updated once during registration. > > (This is a stronger version of __ro_after_init, which is for things > that are only written during __init.) > > AUIU, the goals of the open/close_kernel idea are: > - always inline > - make sure the CPU cannot be interrupted I guess s/be interrupted/take an exception/, to cover stuff like debug breakpoints and such. That might not always be possible, and might not strictly be a requirement, if you can guarantee that taking an exception will cause the mapping to become non-writeable during the handler. That's one approach I plan to look into for arm64, assuming that we create a temporarily RW alias in TTBR0. > - BUG if memory is already writable > - make the memory writable only by the current CPU > - update the value > - restore memory permissions > - allow CPU interruption again > > This makes sure there aren't races with other CPUs to write things, > and that it's harder to use for an attack since with the "make > writable" code is always followed by a "make read-only" action (i.e. > not separate functions that could be used as a trivial ROP gadget). FWIW, on that front we may want to look into reworking the fixmap code. For at least arm, arm64, microblaze, and powerpc, __set_fixmap is a C function that might offer a trivial ROP gadget for creating a RW mapping. Other architectures have that as a static inline in a header, which ensures that it's folded into callers, making it less trivial to reuse. Thanks, Mark.