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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id EFCACC636D7 for ; Tue, 21 Feb 2023 17:34:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8A7236B0071; Tue, 21 Feb 2023 12:34:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8575B6B0072; Tue, 21 Feb 2023 12:34:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6F7576B0073; Tue, 21 Feb 2023 12:34:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 5F3C16B0071 for ; Tue, 21 Feb 2023 12:34:55 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 3B3DB80A1B for ; Tue, 21 Feb 2023 17:34:55 +0000 (UTC) X-FDA: 80491999350.03.564D078 Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by imf23.hostedemail.com (Postfix) with ESMTP id 4AB10140003 for ; Tue, 21 Feb 2023 17:34:52 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=cUKtnnL2; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf23.hostedemail.com: domain of keescook@chromium.org designates 209.85.216.54 as permitted sender) smtp.mailfrom=keescook@chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677000892; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pLxWHkGw8qoawChr7ZMOquDu3/XM9TqD6CJ/g5t4Des=; b=e2YuNFPWkcE9QMp/E9qsXtAXnL9SCOHG0AegrQ4/FElxBMWVkJK4VfOLVUlrQ5828B69Rv abnxjtteyeXNnZufo7jKBZKHND1QNWeeTZuyWrZLeOS6A7RSNyJupKRentqhIAdwJqHWJH eEnlJN2QrczhqmdUMSVSOOeJpHD9vAU= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=cUKtnnL2; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf23.hostedemail.com: domain of keescook@chromium.org designates 209.85.216.54 as permitted sender) smtp.mailfrom=keescook@chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677000892; a=rsa-sha256; cv=none; b=HKnK9a7u589c26LGWVJnkzoQjKBAzevfRBpSKP1TiUWPrnen4cKLdLfru0YM1XmzOfaHT0 auGVgKAytqwMk8ZVLh4JMzpH3Pvl2ggZOvz23yKG1kn+7sg+r6euH3JcoeLoUQqiiE98BP hlmw5lfzhBjZ2YzTjPZv+9goEiSt/4M= Received: by mail-pj1-f54.google.com with SMTP id y2so1572597pjg.3 for ; Tue, 21 Feb 2023 09:34:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=pLxWHkGw8qoawChr7ZMOquDu3/XM9TqD6CJ/g5t4Des=; b=cUKtnnL2Ul2OkVeO8fuJUYUsmzb2eFtPDwmysmRpPUVEAa6Yu+RkHw+pxvct3lZ10L zNweERba3Fs9RnobteImiBWWYyOqi+ZW7ey9SYKDmdb7gqBnBbA6+L4DiRPpzVbjaVn5 C+bPLHlXu6ZVKVWoze3cbaRQVeZqLeD0vPn0o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=pLxWHkGw8qoawChr7ZMOquDu3/XM9TqD6CJ/g5t4Des=; b=uncdRl0YVNZn+W6vtUZ91fFikHSpS21pkmovHGDVOFHlHDUehSRkLnXjnJIbeqhtHE qVSkMYKt7iOJGm4Krt32dcx7sakC/Jft7cV1n5Gaupwqjy+TegImBiBICEYAdLwn6MBK RCBsflKFVshAljBHSrmPsqo7XCDdK7RAwmsUrW5eIgbqvKo+xH9rOv/rQBzgIY9TSYQ6 axLiVrHgXtHXSGtWPoZi8OPRB2JrcxdbgF84Whd5MT/bF1uuRkcGlTnQuZw+KJw088cy 7H66MD495BC7Ap/X6X8VOY/UrkOT07nWQZE81/m+AnOH1VBpmPuGE8Dvld3VDDWFJmXM gLrA== X-Gm-Message-State: AO0yUKVj/h3CkxHpSJJwvt3Q9T/Tmy8CdSc3yI4iiXsySw67b+vaawL9 dNfRqL0UnF8LEu3EVHJtMKpNHg== X-Google-Smtp-Source: AK7set/TfvNthcS1xSc0oxBAX38i183LmXjjSn0X269sbPp8QHwEPMwEmGepSBHlAAQumAk4ZNUzDw== X-Received: by 2002:a17:903:2345:b0:19b:22c2:26a1 with SMTP id c5-20020a170903234500b0019b22c226a1mr6916969plh.9.1677000891107; Tue, 21 Feb 2023 09:34:51 -0800 (PST) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id i3-20020a170902c28300b0019251e959b1sm10110764pld.262.2023.02.21.09.34.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Feb 2023 09:34:50 -0800 (PST) Message-ID: <63f500ba.170a0220.c76fc.1642@mx.google.com> X-Google-Original-Message-ID: <202302210911.@keescook> Date: Tue, 21 Feb 2023 09:34:49 -0800 From: Kees Cook To: Ondrej Mosnacek Cc: Luis Chamberlain , Iurii Zaikin , 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: Re: [PATCH] sysctl: fix proc_dobool() usability References: <20230210145823.756906-1-omosnace@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230210145823.756906-1-omosnace@redhat.com> X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 4AB10140003 X-Stat-Signature: 575gm6pzbxcfrqtgk93z5dcgjcmfmkrg X-HE-Tag: 1677000892-847075 X-HE-Meta: U2FsdGVkX1+4BXe3otGTTXKG8Lj90TcjTN6jbVaicwi1aYvR74xZpSbJOI8KFQd9rUR11cFD5UZaIEWqZfFu3t7CBXx4Lv4OPVw4siWyvqg+GX8ouO5DzX1S/rYnqP3l0FeHJitacZTZPksQHz+uEy6DseKG0KHXiOSQVAIKgZtLikI8xA2rHpg5Wic97eRcZ6L9GH3/g+5x1BqDIyuvMB+OTe+pYjCZpCCQXglBkTn8rTKm0jplq9gbY3GTbBE8sPfoK8wjjMjGnXB57gNlK7YHgVOFkMRSm2xWsn4dP//WLnd34OKhQd1yX19RGED/BqFiV9zaeA44WcMCkNdNzWEU4YMAHxCBtLAXw5xul4DEtDjDidXVMnGvDJ5CHcvtg+UITjDye7XId/oALCQssr3lFcWT9uHzrhycAKcRhH3oKALxM16YEAYOEEjkAPXBMQIlFDjafn7f7RLWMtM5ZNiE/Lp9ULOLrihTanlq+4pkTGEBpPQXts5V1SgPeSyMEQ6SyCqDlQ8HUqbxW5L50Ly2VQZvX9PmOY3WfhxWsGSs2r5YIqPdY9zfun1vS3cOUAT1NrauEbjT/GLdowoS4HVcQpM1vMeSVdFJJPbWwsk+OSdMFWeozKlrHqHEHdKJXnBeyyJx/RMbQjdfsUz5TcZERtWaIVkHVmBs6cpAYOVJl7tH1FP3t6HMSh0jXRNbtKNRG5jQSYawD6/iC3kHamG+zzL6ysp0SNiWZH3R6KVgV3bWi2YKPnDwpv5IZgCiggU+YBL6ZfSoHLUSB/XUHygNAeUbu2vw5bVoVgW9phhVVKWTQgmb//RpC52O353Nc3l5/Uan2DzkVVCKLk5h7MA+uyK0w5USDhlv3EnGN5269JcPPRSiJne73ucxIHJhEV8nGVIl/K5KOCwjxsLig0IuUii+Vxkq1GyDPu87IpisK70IfXXRPzPINMMU5nO1f4kuwq1VR5c2NzsGBen 7ooiXVxe P45NHjyJitBiK2tpeuegwCAwX9OwuF9Dr4cx88wYl5TKc2k5yO2WBTszCH+APVOkilv0fzF9tijv8IFiJkx04GJa9PvkgggP+/5iNhRm4IJzOxGid43I7Ryxbuuzv4N2YGjYIithkLNwlQYOdoXaDSk7LUTqB5eO7I1d2lBC1o1CTIUfctiVaXUVkkGvdn7fsSvFrk0ov/kRpTWGmod/nSs/EBOUxtONbkcKBaTewA2QhSvwnNNUWAin/2xTkEBPUnvbIROoYBPRwPJ+tPHui8LWC7LjCXg14ahU+YUREOrotptTH6P+YxAETt6TweQUPIleIo9hIDdNTKLiC20Djj0ymFTXbtTvlvm84HgSp46OC7Ur5JFn3b0RgLdlF0XveE9cH3e1mrOGsR7luXrxkIRcjOCrknEZFxt+BuBKst0gwrdgKZGKnLwGTuMqPy6kjlGgKgCUUj3+hT+E= 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: On Fri, Feb 10, 2023 at 03:58:23PM +0100, Ondrej Mosnacek wrote: > 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 Ah nice, thanks for tracking this down. Acked-by: Kees Cook I've long wanted to replace all the open-coded sysctl initializers with a helper macro so it's hard to make mistakes. e.g. #define _SYSCTL_HANDLER(var) \ _Generic((var), \ default: proc_dointvec_minmax, \ unsigned int: proc_douintvec_minmax, \ unsigned long: proc_doulongvec_minmax, \ char: proc_dou8vec_minmax, \ bool: proc_dobool, \ char *: proc_dostring) #define _SYSCTL_MINVAL(var) \ _Generic((var), \ default: 0, \ int: INT_MIN, \ unsigned int: UINT_MIN, \ unsigned long: ULONG_MIN, \ char: U8_MIN, \ bool: 0) #define _SYSCTL_MAXVAL(var) \ _Generic((var), \ default: 0, \ int: INT_MAX, \ unsigned int: UINT_MAX, \ unsigned long: ULONG_MAX, \ char: U8_MAX, \ bool: 1) #define _SYSCTL_VAR(_name, _var, _mode, _handler, _len, _min, _max) \ { \ .procname = _name, \ .data = &(_var), \ .maxlen = _len, \ .mode = _mode, \ .proc_handler = _handler, \ .minlen = _min, \ .maxlen = _min, \ } /* single value */ #define SYSCTL_VAL(var) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \ _SYSCTL_MINVAL(var), _SYSCTL_MAXVAL(var)) /* single value with min/max */ #define SYSCTL_VAL_MINMAX(_var, _min, _max) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \ _min, _max) /* Strings... */ #define SYSCTL_STR(_var, _len) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), _len, 0, 0) /* Arrays... */ #define SYSCTL_ARRAY(_var) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), \ ARRAY_SIZE(_var), _SYSCTL_MINVAL(var), \ _SYSCTL_MAXVAL(var) Totally untested. I think this would cover the vast majority of sysctl initializers. -- Kees Cook