From patchwork Fri Feb 10 14:58:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ondrej Mosnacek X-Patchwork-Id: 13135870 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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 36C96C05027 for ; Fri, 10 Feb 2023 14:58:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 66B1B6B0146; Fri, 10 Feb 2023 09:58:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 61A696B0147; Fri, 10 Feb 2023 09:58:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4E2B56B0148; Fri, 10 Feb 2023 09:58:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 3DBDB6B0146 for ; Fri, 10 Feb 2023 09:58:31 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B6E99120784 for ; Fri, 10 Feb 2023 14:58:30 +0000 (UTC) X-FDA: 80451688380.23.23A3C77 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf06.hostedemail.com (Postfix) with ESMTP id 6FD3518001F for ; Fri, 10 Feb 2023 14:58:28 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=OOTSQRut; spf=pass (imf06.hostedemail.com: domain of omosnace@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=omosnace@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1676041108; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:in-reply-to: references:dkim-signature; bh=0GuaSLkqsctnE/R9PtYsrkRTyTfi3C3gicyzl5WAfdo=; b=8CDxyqh0PRASXIr++kOmH5xIgKg8YipP16DCdCFLJcZU0DhBvgmdjkpEhV8zA74wqZpCua C/lvy+yXVVbndX31uQT4HlbjLt/4RlMbep/k2a8LxMQ4dC+haO0ylVjDq1KURy6ILoDhuH KqFFE93sLrTgh75YCoTT2KS/HhgWk/U= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=OOTSQRut; spf=pass (imf06.hostedemail.com: domain of omosnace@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=omosnace@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1676041108; a=rsa-sha256; cv=none; b=yUlyQ3INYij1tCExsDVFITyedeQgPGYhGTd3uSlr3amfp76gFuQH3V2ISvisjOygqafLg0 UF2LgDbXR4lM9J8eDobAjxl/jVu3J080MvfBwRogjarCoyHIProfqgh4MfhGNKPYtw5bKN 8+1sSbJA8/6WcjIArVzk/9D/53cDBjE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676041107; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=0GuaSLkqsctnE/R9PtYsrkRTyTfi3C3gicyzl5WAfdo=; b=OOTSQRutmC5R08n402+OQg92tJNxCr0VZBIq5A6yQs+/01mT5rURB/dbz/oe0+BeAwF6bL W2+arZb2NPGL9V5RuEeX4WF3u0u/x6twQuSUpFpT7DHXmmapDwgYdRsLSuSt7Cs4GiVwTZ DtvFjsMlgr5ivtEkhjl6NnVsEeg7FdA= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-628-ZAszOU1_PWuelCQiFmfgmg-1; Fri, 10 Feb 2023 09:58:26 -0500 X-MC-Unique: ZAszOU1_PWuelCQiFmfgmg-1 Received: by mail-ej1-f69.google.com with SMTP id nd38-20020a17090762a600b00871ff52c6b5so3735615ejc.0 for ; Fri, 10 Feb 2023 06:58:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0GuaSLkqsctnE/R9PtYsrkRTyTfi3C3gicyzl5WAfdo=; b=isfBUMDYdzUsnKT/7qGdy0h7wH1JudkrnxLA6xljAxapSZQJ4VsoyVJyTa39XWejub y3OTpWnicMcI0+A9JfZc0/ygfbepJRDGgRWvLtRAcyAjRM6YJO2itdPoC6P23i7XPsRj nxqamECTrjksIjFY7NjCmjNufcjTV52U9+hDhDJfEJFKUi0ahdcONgWFm2mR+RNv5mU0 RXVoiEGULycjiyqFJnZ4puxpGQWfv2tJKQkCa95AMhlvE6KepHhPbQSi2/+yB4Q3RAep kNojMowbcG3bvb7MHDRv3DIwJeX+9iYcWYqgFjDwKlOVNE9DbTeDQ4QhOpnnn8pe3g2z EPAg== X-Gm-Message-State: AO0yUKVNYMQ9OI9DL8EytJWb4qN/HbKYiCIQImNyUDr9nj9yXL/Fsn8C Ls3opTOpoEhc7V+1YAAkispp6AmlUGU2A4tBzSFERb1l/brqkEy9eUdWeqp5qvrtEzz+xQLpjl5 1ivxpMFUzheg= X-Received: by 2002:a17:906:830c:b0:878:5d46:9ed2 with SMTP id j12-20020a170906830c00b008785d469ed2mr15520866ejx.39.1676041105029; Fri, 10 Feb 2023 06:58:25 -0800 (PST) X-Google-Smtp-Source: AK7set/KjOFaibc3QsEtqV/UTTJ1bP7jy1oQlsDgl+T1vfPoK6IQJs9Fi4kyrR2gjuPttCI2bZZVOw== X-Received: by 2002:a17:906:830c:b0:878:5d46:9ed2 with SMTP id j12-20020a170906830c00b008785d469ed2mr15520849ejx.39.1676041104825; Fri, 10 Feb 2023 06:58:24 -0800 (PST) Received: from localhost.localdomain ([2a02:8308:b104:2c00:2e8:ec99:5760:fb52]) by smtp.gmail.com with ESMTPSA id mj13-20020a170906af8d00b0089b6fd89429sm2509620ejb.10.2023.02.10.06.58.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Feb 2023 06:58:24 -0800 (PST) From: Ondrej Mosnacek To: Luis Chamberlain , Kees Cook , Iurii Zaikin Cc: Greg Kroah-Hartman , Jiri Slaby , linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: [PATCH] sysctl: fix proc_dobool() usability Date: Fri, 10 Feb 2023 15:58:23 +0100 Message-Id: <20230210145823.756906-1-omosnace@redhat.com> X-Mailer: git-send-email 2.39.1 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 6FD3518001F X-Stat-Signature: 7udz8wsuraqzjj41yixzgzgipqjusr5q X-Rspam-User: X-HE-Tag: 1676041108-746685 X-HE-Meta: U2FsdGVkX19GsSs8F9MZlzv9r0NBmnEOknCaETI026nf5NJHQqhfFwpOEBXfewtHwP2R3m3WmRDH9poKx0hcdNzVrYvFym+ngjEFrwjZQPehsImryNywu4+YZp/YOXzPfcdl/BaK+yhYa/RjCZ5JoATSp/msdchbWCAbR7Nip2ZVnefPECB/Jo1QjAiXyg0gfuMgN/pvG8oEVB7NsdrVuMvGpHf6WYSWkZQ1BwwVWcwuTpqc+Z3r9WlVxz+AkH+oiP3Ye4Q3BTQ0j8keEJukF6Wv5zujcRRNHPdX3TGLc50gXtwPlTvAJxuc2yIX7MnGk0YtpeOBfxofiN+6SrShQUyAqyX0vPLvyO4U0yYlsI1lWZK2C2bP1Zs0WFUzt3yoRnXZf+HroXnGxyIhSFnZHMSX7r82MuL8PNNCHT77T+/9jQW7YzdStO+6Ovrc9xkqZ64nGWIDHNuJtRusis1CS+oXHiowajpgJyDZK8sDoIUvkuEbyvTt+JmBw59eI51TTI/Yz0dJ2A5tbiH1XDqMZqF+KAWoCxNfouBKjlIlf4Unt9sejmOKOmeSbZXv3YVECgW8RI9p+z10hg9wjhbofjTk89qsVT7ERXxsEDa8Da09C34Uir8N4O+OvG+O9F+kacePSZvQFNfqPL5DPxJy0spMDv3Tq51fUGjgmuP/2XUOdeudOqW4EqGepFf4bKbdi3mZ60DrokkMHWABoUcvBmUHrjGFq/sIS3cM9lWcCifTGoxOa0lLDmasudeZAJHijs73N/cnzoebupZnXVtu8Zo6ORUBVru3qK1/Kf8/xNgkhOSVSdPAC7C5cHPT89ghR8htQ92ZtwxJh++mcKZXXaXw/26BouWEE1WLirDuhkVEOgZ658kR0fcmCddpscWhFPztRLRFdf+N+doPdA6xpu5XBD6/uWFIkVBniNRi5ykX1IUNTDGaqSj4gqj4IAqdNF/VhJLVX/jKT3jau+N krylV188 mOjVnVtWd64bWntHgqMnksAfdDufdt5YmIO8jC0Q/u4sOSBaOdrWCW/lG9ypBOKlvBXHdJnTwkVScEBBRBpqVKopyjTDIKaDRZ4tF7x07+lyXVNUtLf1Ckr2DK7/caBiUsWWAWgjdEm++L9bDPPxnh+HdPr30egMeumkha8FHcbUaxESRFhRt8v0OjH24rs0nax1TfzlymNLlI7Posj6/ZXWTVdti33QedoOFjOGPvTQhkgoxtkNfXwTdrDdHoe18QqfyCiB/kpvY3/u/c+tDxZd1Ojp6F6P4gS0xHW09E1qucXhCInaW5cyrFxN3SxWnPKsiHLJEiRv9lgSLGR6yQuxh85/icW9Kjcu1Xz1+U5PkRB6B6L6zCZBZaBbFkYHg/DUnTTP9iGNLHG2r7LyFg/FjR0R8BmfrgyYr6oaM8zX6cDk5O2jToaWsEA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) in table->maxsize, because it uses do_proc_dointvec() directly. This is unsafe for at least two reasons: 1. A sysctl table definition may use { .data = &variable, .maxsize = sizeof(variable) }, not realizing that this makes the sysctl unusable (see the Fixes: tag) and that they need to use the completely counterintuitive sizeof(int) instead. 2. proc_dobool() will currently try to parse an array of values if given .maxsize >= 2*sizeof(int), but will try to write values of type bool by offsets of sizeof(int), so it will not work correctly with neither an (int *) nor a (bool *). There is no .maxsize validation to prevent this. Fix this by: 1. Constraining proc_dobool() to allow only one value and .maxsize == sizeof(bool). 2. Wrapping the original struct ctl_table in a temporary one with .data pointing to a local int variable and .maxsize set to sizeof(int) and passing this one to proc_dointvec(), converting the value to/from bool as needed (using proc_dou8vec_minmax() as an example). 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. 4. Fixing the proc_dobool() docstring (it was just copy-pasted from proc_douintvec, apparently...). 5. Converting all existing proc_dobool() users to set .maxsize to sizeof(bool) instead of sizeof(int). Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") Signed-off-by: Ondrej Mosnacek Acked-by: Kees Cook Reported-by: Storm Dragon --- fs/lockd/svc.c | 2 +- fs/proc/proc_sysctl.c | 6 ++++++ kernel/sysctl.c | 43 ++++++++++++++++++++++++------------------- mm/hugetlb_vmemmap.c | 2 +- 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c index 59ef8a1f843f3..914ea1c3537d1 100644 --- a/fs/lockd/svc.c +++ b/fs/lockd/svc.c @@ -496,7 +496,7 @@ static struct ctl_table nlm_sysctls[] = { { .procname = "nsm_use_hostnames", .data = &nsm_use_hostnames, - .maxlen = sizeof(int), + .maxlen = sizeof(bool), .mode = 0644, .proc_handler = proc_dobool, }, diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 48f2d60bd78a2..436025e0f77a6 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1124,6 +1124,11 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table) err |= sysctl_err(path, table, "array not allowed"); } + if (table->proc_handler == proc_dobool) { + if (table->maxlen != sizeof(bool)) + err |= sysctl_err(path, table, "array not allowed"); + } + return err; } @@ -1136,6 +1141,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table) err |= sysctl_err(path, entry, "Not a file"); if ((entry->proc_handler == proc_dostring) || + (entry->proc_handler == proc_dobool) || (entry->proc_handler == proc_dointvec) || (entry->proc_handler == proc_douintvec) || (entry->proc_handler == proc_douintvec_minmax) || diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 137d4abe3eda1..1c240d2c99bcb 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -425,21 +425,6 @@ static void proc_put_char(void **buf, size_t *size, char c) } } -static int do_proc_dobool_conv(bool *negp, unsigned long *lvalp, - int *valp, - int write, void *data) -{ - if (write) { - *(bool *)valp = *lvalp; - } else { - int val = *(bool *)valp; - - *lvalp = (unsigned long)val; - *negp = false; - } - return 0; -} - static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) @@ -710,16 +695,36 @@ int do_proc_douintvec(struct ctl_table *table, int write, * @lenp: the size of the user buffer * @ppos: file position * - * Reads/writes up to table->maxlen/sizeof(unsigned int) integer - * values from/to the user buffer, treated as an ASCII string. + * Reads/writes one integer value from/to the user buffer, + * treated as an ASCII string. + * + * table->data must point to a bool variable and table->maxlen must + * be sizeof(bool). * * Returns 0 on success. */ int proc_dobool(struct ctl_table *table, int write, void *buffer, size_t *lenp, loff_t *ppos) { - return do_proc_dointvec(table, write, buffer, lenp, ppos, - do_proc_dobool_conv, NULL); + struct ctl_table tmp; + bool *data = table->data; + int res, val; + + /* Do not support arrays yet. */ + if (table->maxlen != sizeof(bool)) + return -EINVAL; + + tmp = *table; + tmp.maxlen = sizeof(val); + tmp.data = &val; + + val = READ_ONCE(*data); + res = proc_dointvec(&tmp, write, buffer, lenp, ppos); + if (res) + return res; + if (write) + WRITE_ONCE(*data, val); + return 0; } /** diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 45e93a545dd7e..a559037cce00c 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -581,7 +581,7 @@ static struct ctl_table hugetlb_vmemmap_sysctls[] = { { .procname = "hugetlb_optimize_vmemmap", .data = &vmemmap_optimize_enabled, - .maxlen = sizeof(int), + .maxlen = sizeof(vmemmap_optimize_enabled), .mode = 0644, .proc_handler = proc_dobool, },