From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 317D1C04FF9 for ; Mon, 15 Apr 2024 17:05:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=t74Ha83J2N9JWsXP5xaKP7dZh9L1kD6YhfeTILdupT8=; b=iuxv641VtXymzM HMbdWdNUrexzynFU5WNuvF1qydEhZ3rZ62as2TSK3Fp9tMtqOkzQTH77PwgQ102D7q49QAn5wZmm1 6BQ4jERFWZn/fHWusyR3gQ1ToEn+ARYTMkdheCeO2DDU5OE2DevHJZwOIRLnedqMJDwE2IWRKVhlV RCGjMITr+MzMVmrAJttBYr6A29zPGaWcPuLORoqYhFSZN8klOdP/X/dCJ+p8LQPnbqcNKBqKzfErw VZhxsSH6xmoU9n4aye0yU4VsyldEJoUlzMfYQdl1XWeSunq/T31BF3lAeWMDCXkx7vIFcil8qPKIQ LdXICRvIErv20yIoUB+A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rwPl3-00000009DXZ-3mUF; Mon, 15 Apr 2024 17:04:41 +0000 Received: from mail-oo1-xc31.google.com ([2607:f8b0:4864:20::c31]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rwPjD-00000009C1I-1XL9 for linux-arm-kernel@lists.infradead.org; Mon, 15 Apr 2024 17:02:54 +0000 Received: by mail-oo1-xc31.google.com with SMTP id 006d021491bc7-5aa17c29ba0so2053234eaf.3 for ; Mon, 15 Apr 2024 10:02:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1713200565; x=1713805365; darn=lists.infradead.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=TJWFLlk6ICNRWZIRD9WFPl1zIlLjuJU9kdqkQByqVPQ=; b=KtkU7UrhKbee5Z8bIa1K4eZl7dVlLVqmCrINIq+Hx4o+p9r3ifuSgCz8A6Vat76wDi OzwhypxxvKYovHclguWtPOUw8MrL3ryeyHg2chVmDtKG1NSMQsyBsSBL6CJJMU+DIkWW CSmXzxnKAu05HisTHV2TLrA8Tcp/L8Lh5Gbuw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713200565; x=1713805365; 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=TJWFLlk6ICNRWZIRD9WFPl1zIlLjuJU9kdqkQByqVPQ=; b=i8GuZ1uKOIVA9h4iLwdBnWKoC6xwiDSU37JF8GYa51d+4Jbaap4+0h/TrCA/LDJxt+ FeJ4N4RMjiiMAFm3NMAeCBs5jgHDW5OEh4Ptm1dqDRI0c71Ti/SGLLlBkSl0KQHWe4vc Iwc35p8tOG7mOdkvR0FTKmHdh8Ti94HgcgDLUTnh+YYkwUdyUmQwr2C6PnKAMVgYNboI BZrg6r72LAYH1dG/k+j3l00XnuAMPOcv6G7TqqqRQq62lOG0PYLj5kuvr2qWMNwpRIFi Jf75O7OU25yZMqgxy6513wrd9zCb6/2cyL24c1XSUa6JCxUnCBx9pPnZw8uc8BkWPyEt 5SfQ== X-Forwarded-Encrypted: i=1; AJvYcCXkhu9up4uJ/VFMy0i91Gf+SQXOpX1o3X1Pvy+qaycpw2RogTSNOBgQDzyhdpVlktaL4fQV3sFr/zb1EsZGnR+6G7wXYQpvL7HLmxNBI9D9jp2zrF0= X-Gm-Message-State: AOJu0YyJjyjzTwHaXy9bB0XbRXGRKiP2aBuZr++pi5Jiw2tv0Aqf8Km3 uJ/yTWKOXvOsL8KMVhvbeUZkJ/D9LzXMM08m0G2fMmYtChKk4PxfBPk049p2TfSy/tyQV9T2sl4 = X-Google-Smtp-Source: AGHT+IEYOZbcVvtVCv2aHOV1jRW+BNuqsUZsbfmYxla8sMqqORfgbGkTG2HFnkY4yJ0uy+SrQBOPFw== X-Received: by 2002:a05:6358:688b:b0:17e:bfb8:dd86 with SMTP id z11-20020a056358688b00b0017ebfb8dd86mr11908320rwh.3.1713200564920; Mon, 15 Apr 2024 10:02:44 -0700 (PDT) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id q4-20020a656244000000b005dc4fc80b21sm6296328pgv.70.2024.04.15.10.02.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 Apr 2024 10:02:43 -0700 (PDT) Date: Mon, 15 Apr 2024 10:02:42 -0700 From: Kees Cook To: Mark Rutland Cc: "Russell King (Oracle)" , Tetsuo Handa , Linux ARM , syzbot , linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com Subject: Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set Message-ID: <202404150945.0E96AB727@keescook> References: <0000000000004cf5c205faf1c7f3@google.com> <2ab55fd1-7eb0-488e-93ea-660fa05ee43a@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240415_100247_777572_E390448C X-CRM114-Status: GOOD ( 43.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Apr 15, 2024 at 12:43:59PM +0100, Mark Rutland wrote: > On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote: > > On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote: > > > On 2024/04/15 18:44, Russell King (Oracle) wrote: > > > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: > > > >> On 2024/04/15 18:02, Mark Rutland wrote: > > > >>> 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > > > >>> > > > >>> That commit says that all accesses are bounce-buffered and bypass the check, > > > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was > > > >>> wrong or the user_regset_copyin() code has changed. > > > >> > > > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? > > > > > > > > Have you visited that URL? It doesn't point to an email containing a > > > > patch, so sorry, I don't know what patch you're referring to. > > > > > > > > > > Containing a link to a diff. ;-) > > > > > > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c > > > index c421a899fc84..347611ae762f 100644 > > > --- a/arch/arm/kernel/ptrace.c > > > +++ b/arch/arm/kernel/ptrace.c > > > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target, > > > const void *kbuf, const void __user *ubuf) > > > { > > > struct thread_info *thread = task_thread_info(target); > > > + const unsigned int pos0 = pos; > > > + char buf[sizeof(struct user_fp)]; > > > + int ret; > > > > > > - return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > > - &thread->fpstate, > > > - 0, sizeof(struct user_fp)); > > > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > > + buf, 0, sizeof(struct user_fp)); > > > + if (!ret) > > > + memcpy(&thread->fpstate, buf, pos - pos0); > > > + return ret; > > > } > > > > > > #ifdef CONFIG_VFP > > > > No, not unless there is really no other option. It's hacking around the > > issue, creating two copy operations of the data (one onto the stack) > > rather than solving it properly - and I will not put up with that kind > > of mentality - it's a completely broken approach to open source > > software. If there is a problem, always fix it using the correct fix, > > never try to sticky-plaster around a problem. > > > > It seems there is a way for architectures to tell the code what is > > safe to write to, and it seems that a misunderstanding meant this > > wasn't implemented. So let's see whether it's possible to fix that > > first. > > I completely agree. FWIW, the bound buffer approach is used in other places as well (especially for places that are not fastpath, like this case), as it creates an explicit bounds check (i.e. the buffer on the stack). But yes, if there is a way to specifically allow this during the kmem setup, let's do that. The timeline on this is interesting, it first showed up on syzbot about a year ago (2023/05/05 12:53) https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14 But hardened usercopy (and commit 08626a6056aad824) are from Jan 2018. No one noticed for 5 years? :P > We'll have to wait for Kees to wake up, but IIUC one assumption here was that > thread_info was particularly sensitive, and hence any state to be copied > to/from userspace would live in thread_struct. Either we need to remove that > assumption, or we need to move things so that we can use > arch_thread_struct_whitelist(). > > Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly > mechanical change to move fp_state (and vfp_state!) into thread_struct. That > would mean that the TI_FPSTATE offset would grow, but assuming that still fits > into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist() > could be used to handle these structures. Sure, or use a bounce buffer, since no one noticed this for 5 years it probably isn't heavy used[1]? But sure, I'm open to whatever -- the point is to not arbitrarily allow usercopy to touch the memory here. -Kees [1] We probably can't remove PTRACE_SETFPREGS, but yeah, it's really uncommon: https://codesearch.debian.net/search?q=ptrace%28PTRACE_SETFPREGS%2C&literal=1&perpkg=1 - systemtap test suite uses it - rr is x86 only - crui doesn't have a arm backend - python ptrace may use it - edb-debugger is x86 only - llvm mentions it in comments only -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel