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 X-Spam-Level: X-Spam-Status: No, score=-13.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4330EC4167B for ; Tue, 8 Dec 2020 10:37:39 +0000 (UTC) Received: from shelob.surriel.com (shelob.surriel.com [96.67.55.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 68E4D221F7 for ; Tue, 8 Dec 2020 10:37:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 68E4D221F7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gmx.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kernelnewbies-bounces@kernelnewbies.org Received: from localhost ([::1] helo=shelob.surriel.com) by shelob.surriel.com with esmtp (Exim 4.94) (envelope-from ) id 1kmaMU-0002Tw-HI; Tue, 08 Dec 2020 05:36:50 -0500 Received: from mout.gmx.net ([212.227.15.15]) by shelob.surriel.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1kmaMQ-0002TQ-B8 for kernelnewbies@kernelnewbies.org; Tue, 08 Dec 2020 05:36:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1607423804; bh=a9Xn57ltEob8KxLt5FrD2vD1kirOJHAXfULlBDW3q0Y=; h=X-UI-Sender-Class:From:To:Cc:Subject:Date:In-Reply-To:References; b=VhrIRab5tYZGL/h+EG/AHwdnNdw+ZBuOMBarsXP35LxuS53tHgF4Xeh80tDe1Ie7V 9y5wHszX5duugSesTzXPDvCJ6r5s7TGCbBUlIozSyHhB50fcKHFtmJLSzRrcb1cdi4 y6LsE7UHkNE2LIcox7+ZIVIIo2IHdNrMR61lYLdU= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from localhost.localdomain ([83.52.231.213]) by mail.gmx.com (mrgmx004 [212.227.17.184]) with ESMTPSA (Nemesis) id 1MDhlV-1ktUEu2itr-00An7t; Tue, 08 Dec 2020 11:36:43 +0100 From: John Wood To: kernelnewbies@kernelnewbies.org Subject: [RFC PATCH 2/2] security/brute.c: Protect the stats pointer Date: Tue, 8 Dec 2020 11:35:57 +0100 Message-Id: <20201208103557.6471-3-john.wood@gmx.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201208103557.6471-1-john.wood@gmx.com> References: <20201208103557.6471-1-john.wood@gmx.com> MIME-Version: 1.0 X-Provags-ID: V03:K1:UODZDm9soNWyrBD7bWJHeVXNjTcO6DONNzBzv574Hac58wGCPZG CG1NDdRSdOPdi6ng4zCKRlubPMkPl4zL0cSo4/jyMGyqQIP7iYyh7eKuCKh7eVbVWaSxM5F L5bbLQE2TN42i1qKgRanLvvwAhgHaNHBaHQweOAKivdoG5wekK25m0Bx0nzm9SiLrRp4neJ X8x0dvmUuKU5OfMPIPZpw== X-UI-Out-Filterresults: notjunk:1;V03:K0:WWv6Ga383CU=:VGQXE6E07oSqjMja7OKYct iPQGdkqaybwGM751JNcPTtDGvvQ/9yOuyXtjaoxHu/kfTWH/bX5Ql79qDiIM4r2EOh3YbGaWD djavzUzsW9HMSNE9ijfiziMqqRZSscjpOQldHLItoS+LgiH2Yty8t22VwYfBunkaM/WcXYjGl budrRa6ry53KA2jZVVD8bsio8SaBfHVrOqv99wSbc+OoBiRlfYdZUr9Zxb9RHxGcfyaOgQKKo TDqTyoAZqG3/vTZy7+pL+NUwxqH6GroW62T0iQq+QPVTd2iCvlZTz4z3mOiw5BC4agsdYSBfn dMTNK+n3oG9+WdtPhzfgA+l9ILoJa4g+UB+0KHsS5LXpEM/FwKxb+CiHPMAtOghYBKrgtIT61 pjrHAeQNGWEnrr6tfGeLFzwmKm32KA1SySptR+maL5gbhEjn2o8iiaUcKist2NlPEvXWUWrUz 7twALY4WWbg3lV024JN2Ha9FmytAC79Y/8/eVVPSbKdLEpq5tNCAb3DNezzAEUy61MkmLgskJ VZm2lWNdUgg2utiFETwX2zkrpweFgfGT25lbGnLCFszyiMKHNgah0/HZhwWFlpPukrplSyPYX tI97SmdiA2jw2S8KhHn6YRFotoHimD9nNUnY+lvx3uGXu2bTZDmgjG8498u1HfnhrFi0QAiCf 2tklyDD5hAKbF8z/RVaIf9XWg9ma6qkq4rG2VykJVmdskKO91X4Sg1f7VNlexBsX+5XGGsAyI ybzDJMjJrsXypzOuapfH1ggcgEQPo3Ikyws3lv9ygawRBhtSSha97Ruua30Xp/k83xJ0/MMT7 tuH9hU5hdk6kqipZUlHAUsiBdnj5+eENaHcFINWmqm3Cm5sbakIlpri3ev7gQ3l03cMRLNh6K S6EyuX1/TjPWzqz4bo0Q== Cc: John Wood , keescook@chromium.org X-BeenThere: kernelnewbies@kernelnewbies.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Learn about the Linux kernel List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kernelnewbies-bounces@kernelnewbies.org I think the stats pointer present in the task_struct's security blob needs to be protected against concurrency for the following reasons. 1.- The same process forking at the same time in two different CPUs. 2.- The same process execve() at the same time in two different CPUs. 3.- A process forking or execve() while we traverse the fork hierarchy in the brute_get_exec_stats() function. 4.- More cases that now not come to my mind. So, the question is: Is the locking used correct? I use a read/write spinlock to allow multiple readers when the stats pointer not change. But when this pointer needs to be updated, the access must be exclusive by only one writer. Moreover, the kmalloc() uses now the GFP_ATOMIC option to avoid sleep in an atomic context. Is this a problem? It would be better allocate the memory for the stats structure outside of an atomic context using GFP_KERNEL and update the stats pointer inside the atomic context? This will make necessary always allocate memory (and free it if its not necessary) in the task_alloc hook. Also I would like to know if is it necessary a more fine grained locking? I think that it is not possible because we need to avoid that, for example, if the same process is execve() at the same time in two different CPUs, that two stat structures are allocated and then only one assigned to the shared stats pointer. I think that in this situation will be a memory leak. I also believe that the other cases cannot use a more fine grained locking since we need that this sections are atomic to avoid inconsistence states like the above commented. Another question. If I use this lock to protect the stats pointer, can be the stats->lock avoided (to acquire and release) in the brute_share_stats function and the task_execve function? I think that in this scenario the brute_stats_ptr_lock lock also protects the access to the internal data of stats structure. But I think that the stats->lock cannot be removed completely since in the task_fatal_signal function the brute_stats_ptr_lock is acquired in read state but we need to write to the stats structure to compute the crash period. Any constructive comments are welcome. I would like to know your opinions about my locking system but remember that I'm a newbie :) Signed-off-by: John Wood --- security/brute/brute.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/security/brute/brute.c b/security/brute/brute.c index 60944a0f8de8..926bffb61a3b 100644 --- a/security/brute/brute.c +++ b/security/brute/brute.c @@ -18,6 +18,8 @@ #include #include +static DEFINE_RWLOCK(brute_stats_ptr_lock); + /** * struct brute_stats - Fork brute force attack statistics. * @lock: Lock to protect the brute_stats structure. @@ -74,7 +76,7 @@ static struct brute_stats *brute_new_stats(void) { struct brute_stats *stats; - stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL); + stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC); if (!stats) return NULL; @@ -135,17 +137,22 @@ static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) stats = brute_stats_ptr(task); p_stats = brute_stats_ptr(current); + write_lock(&brute_stats_ptr_lock); if (likely(*p_stats)) { brute_share_stats(p_stats, stats); + write_unlock(&brute_stats_ptr_lock); return 0; } *stats = brute_new_stats(); - if (!*stats) + if (!*stats) { + write_unlock(&brute_stats_ptr_lock); return -ENOMEM; + } brute_share_stats(stats, p_stats); + write_unlock(&brute_stats_ptr_lock); return 0; } @@ -177,8 +184,12 @@ static void brute_task_execve(struct linux_binprm *bprm) unsigned long flags; stats = brute_stats_ptr(current); - if (WARN(!*stats, "No statistical data\n")) + write_lock(&brute_stats_ptr_lock); + + if (WARN(!*stats, "No statistical data\n")) { + write_unlock(&brute_stats_ptr_lock); return; + } spin_lock_irqsave(&(*stats)->lock, flags); @@ -188,6 +199,7 @@ static void brute_task_execve(struct linux_binprm *bprm) (*stats)->jiffies = get_jiffies_64(); (*stats)->period = 0; spin_unlock_irqrestore(&(*stats)->lock, flags); + write_unlock(&brute_stats_ptr_lock); return; } @@ -195,6 +207,7 @@ static void brute_task_execve(struct linux_binprm *bprm) spin_unlock_irqrestore(&(*stats)->lock, flags); *stats = brute_new_stats(); WARN(!*stats, "Cannot allocate statistical data\n"); + write_unlock(&brute_stats_ptr_lock); } /** @@ -210,8 +223,12 @@ static void brute_task_free(struct task_struct *task) bool refc_is_zero; stats = brute_stats_ptr(task); - if (WARN(!*stats, "No statistical data\n")) + read_lock(&brute_stats_ptr_lock); + + if (WARN(!*stats, "No statistical data\n")) { + read_unlock(&brute_stats_ptr_lock); return; + } spin_lock(&(*stats)->lock); refc_is_zero = refcount_dec_and_test(&(*stats)->refc); @@ -219,6 +236,8 @@ static void brute_task_free(struct task_struct *task) if (refc_is_zero) kfree(*stats); + + read_unlock(&brute_stats_ptr_lock); } static const u64 BRUTE_EMA_WEIGHT_NUMERATOR = 7; @@ -313,8 +332,10 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo) u64 exec_period; stats = brute_stats_ptr(current); + read_lock(&brute_stats_ptr_lock); + if (WARN(!*stats, "No statistical data\n")) - return; + goto unlock; brute_get_crash_periods(*stats, &fork_period, &exec_period); @@ -322,10 +343,12 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo) pr_warn("Brute force attack detected through fork\n"); if (fork_period == exec_period) - return; + goto unlock; if (exec_period && exec_period < BRUTE_EMA_CRASH_PERIOD_THRESHOLD) pr_warn("Brute force attack detected through execve\n"); +unlock: + read_unlock(&brute_stats_ptr_lock); } /** -- 2.25.1 _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies