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 4C662E77188 for ; Thu, 9 Jan 2025 02:15:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D89796B0083; Wed, 8 Jan 2025 21:15:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D43656B0085; Wed, 8 Jan 2025 21:15:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C00FE6B0088; Wed, 8 Jan 2025 21:15:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 9F4536B0083 for ; Wed, 8 Jan 2025 21:15:44 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 2DF9E12173B for ; Thu, 9 Jan 2025 02:15:44 +0000 (UTC) X-FDA: 82986297408.17.84E7BEC Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) by imf02.hostedemail.com (Postfix) with ESMTP id 23A7C80003 for ; Thu, 9 Jan 2025 02:15:41 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=BdrqXt1A; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of isaacmanjarres@google.com designates 209.85.214.174 as permitted sender) smtp.mailfrom=isaacmanjarres@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736388942; a=rsa-sha256; cv=none; b=PvpulBRY9c0nR1kLCl2hyZSed+o5b2t9/5H0Wc2ypNqgP5//V9shVdeiWqdUJ0Ya/71DvA 1eFCfEPjuzTkFnjuObXEZYEGjWDchOIrZVoy62o6MNwSbQ9Yea34F3o8vm2OhWGkjosnud 6LEsb9JIC96wOzLt3mMh5tFLdQ9Xeqg= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=BdrqXt1A; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of isaacmanjarres@google.com designates 209.85.214.174 as permitted sender) smtp.mailfrom=isaacmanjarres@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736388942; 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=u3oGaMxnaFjobvsXNOp6XPItf3NxrOmw9ZGL9171jVY=; b=WPy6O7qtzzoXwdDJULzkFuUgn5YntHPHKJxq96OJA9Lox+0Iku1RADYVxjFs1ybJj1v1A0 gLfmyokn5qiaV2nN0JON7yWiT7If8Ileg0IPNcX3tGijWrdQcYJJrA0D/XBfj/pUOQkxya yDGQlBLsCK7qPA5KGtd1XilgsmWGVLg= Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-215740b7fb8so73725ad.0 for ; Wed, 08 Jan 2025 18:15:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736388941; x=1736993741; darn=kvack.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=u3oGaMxnaFjobvsXNOp6XPItf3NxrOmw9ZGL9171jVY=; b=BdrqXt1AGkkcYibhdDpWsAYvsk7EG9uly8vttDryVqIS0A3mI+bG1DDKIJLnLhPgeO 4O8bz+Wd/31cxG2SXDIsLhTR7mkPRsiySodUXNPbZ7vWYPYdBfDnb5Cba0NEVqzzeV4c JUyQ0P5V5+r65gb1M4HMKd7UKJxmUnM3GRcpCR32HoO0xy++HHqvCuNkHpRyYR2McPyd JmdSwEfFmASdp3wUrY5lghg9RdaDzOk+omvQsbkl3NgR9tRnEv4c8Ocr2/9KSjJDUQFA xcrMh28S6NErvVsEQcj+F9Gm+dyQYEP0tjsnKLQhP4JdBtpW3DPFRiw8dcnoMMhJIdlk YqlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736388941; x=1736993741; 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=u3oGaMxnaFjobvsXNOp6XPItf3NxrOmw9ZGL9171jVY=; b=jUQaUs/+0vufPVf39chxrFM6UguFqjGsOoU90lVBH9lBt/hnl9oE2CuTnvbSwQcgD+ WapjWCnGDoZ9+pm8vv10mvHgiN4mHwhGNh+thKUJLTwj0fYuZRNBKimlyhvMlJEkLF4c vq+PllvSBMtj2pmquSX/EKi79bhU+dy9V+hASaS4hS32IawVrBqrhBwAvRRJgfWY0Aa9 qq4ehMEi1L/qALSdFh/RiC6E0bYFAjMtm2lrE9huNVrVcJUkamcAeHRLC/abfFv217lz 5sQIbd7igNXHhXNh5nyJcqQVSlHDz4KUwaYE5KxfIm3tkBp2SwA2vPO0XncWy9hzP/mh 3Ing== X-Forwarded-Encrypted: i=1; AJvYcCUOpuoA3HHd/Kje+3dFld/XwJSqsYm6xzFQRkh9HHrjv/x/fCUpzJ/UejfpSObRgzqsdJSk1Ma1FQ==@kvack.org X-Gm-Message-State: AOJu0YzH4YjBVR19jTsZ+4Rw4ZF9g3KzYQUTOONtAalyiQ8ctyu1CC+T zTU9b9Xr8AWfIHDJ0Bk6ossAbugPDmSlZknECMBoDQN8j31Dk9J/Ia9TZ5IaXQ== X-Gm-Gg: ASbGnctUHRI4jfjUF6C/l92Uw0OyeqTMyK7N9V7Cvusc7gLp8Fvdah9PzcNoWbBROgi yMCJUG6EXZOsr2fB9Z6Cxp8UP7zNG2/oQM1OJXuaGfwBB4UT8srx2gvLe8kl7wNV1ITmx16g4qe YsBA9rCIvDlmcufaaEgFwPKUaChFzd4MM+0/t7ykRR8LufJGJfmi9SC15s6+ANZ82PSkKMCMnhT pMW70XbMT6wEDuE1ETdrpd37wZYIE6Q7EUpnw+sAOo+apUuKXnR/TkZIg== X-Google-Smtp-Source: AGHT+IHp1UDFH1wYfSby/luLK1JrWrYxwsSQ2PilxTKE2uO5KN8tmhE5qCPW6lpSn+HQVr96gKqulg== X-Received: by 2002:a17:903:2601:b0:217:8612:b690 with SMTP id d9443c01a7336-21a8ea0f4d3mr1144625ad.8.1736388940541; Wed, 08 Jan 2025 18:15:40 -0800 (PST) Received: from google.com ([2620:15c:2d:3:e514:4b22:2740:5ec1]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72aad831148sm35941023b3a.53.2025.01.08.18.15.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Jan 2025 18:15:39 -0800 (PST) Date: Wed, 8 Jan 2025 18:15:36 -0800 From: Isaac Manjarres To: Lorenzo Stoakes Cc: Andrew Morton , kaleshsingh@google.com, jstultz@google.com, aliceryhl@google.com, surenb@google.com, kernel-team@android.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name Message-ID: References: <20250107184804.4074147-1-isaacmanjarres@google.com> <20250107184804.4074147-3-isaacmanjarres@google.com> <4291d9f0-4483-40e5-a54b-d006eb52c8cb@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4291d9f0-4483-40e5-a54b-d006eb52c8cb@lucifer.local> X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 23A7C80003 X-Stat-Signature: pfd94mu3m874p6iww5yftgj78x5f7gj8 X-Rspam-User: X-HE-Tag: 1736388941-310015 X-HE-Meta: U2FsdGVkX1/Xti9OhyrUPnDZ4jpT/73toeyDECjhk/H6UjMWAI+6KlwLESyn1KFOcjJVUNK77eyyvRg+pR6FTjTs3qlMHToUsLQ4tGcNYWzyC55LAuziWkgyTE72+SJcpRVbRU3aYnImkTHy9Bz6HrZ9vtfE7VueLp3cE3r2VnMgJuRC0Y5VgkpNlxSZjMK46oTJoO1RBb7H7a0WOe+gukzp/pXIrbZ45O/B0nBGdnFvya2pXCwHLsrIGygcFLenjajfqxR0EC8rR/wlIzv/iHmitlHkyAXDWrBnJwg7Vwxoyz9Bv6Y+EccwPLEr97t4IkpDvS+W8I2tssFqUbhBeu5QQeyFoMzINgZjwbgLJmzUECvbQFczIz5nhqJnFPFC7ls7DVnJKOrSgLP9SILdR9hvw0e/01+39LxbO7TXV5roWF+LvmuenW7uVi1JJGw6hLgJh0OABvgkGCDvXpFQIDszuGfQtj01uH40U6mggVvcSWnt15VGdW9zJjQJSr368ZhGaIT8gPYS/SfS46K8wPEgqhqrtZMNhOZ6EAy2bC7+PL/GVQo8VCK4mmBhcPPvk3DBk/bgkQxn4RQLR2b9vuhlthDlrW2jLV+CM57jIJMp9d6dgl6Xrc2Zpt8qiAA1S+tt/9VEDlBtLX3uH2IGi8cbEnIKF1HMrWaH4V9b2Yybv0NVpkJgLTYqt7TnGAPT7ERzEy+pp10h+C9ll3bVRBTkXhpjw/GR8xntIEa0YO1KAhlLYgcVT+E9lZGTw0ogxzEGKaJSrHqTi2enX6x3YoXXubYLb6yqdTCtNLlhK0ZdopXqvRH3fJJteqzNdH8nZ5P21b9hoyTPgpTrxsrND0rR8EwfqSdUYv1GbvxbzmyRkW8H/JCTOjcKHQ37DSCi3/Te5g/vNv7mgqtqMiZ5pc1UruBW2Cv8omJpSiXTJvgTWQJ898qBa1ADgwslzE96/vlFHyUXmAvN7VZ1Ur1 rkh4CDuy L8ynUvvsIFrBBCzYe6Gbv75XVG+PRSTuVICHAto6n3T8o+Tmc+LHztyH9CDRLsyEiy45VuiTAu5mJ+QSHqZx5/b5jV52rDU1ZeqH4Lw00V/SGyGRVU3nFN+AQEgxz2k5TZT66lV+zWmfVeqslUw4Y4Bo5sqgFaFtXOjB14dY+jF5XDfVD92iHxC9GBm3t9Dp1CH9u3SN1GMvxAQAwF0ymmWYxx6+wXjseC898w1FMDZl/yOiZx75BAtwE4qv37QVgmlgN/shKdRn7d0xl0xTn8wPHyedYoN+mOpSdJonr/xgMVT0HkUHDjYR1Ycl7DTUk5w7XM1fTxAHvIvSlpECa2AVbYQ5BPSDxRWjh6aeoXrJm4QvBIV5xAr9rcyfWQ5DvZ9Km39QyxL7P4Twh9pTCtcjCcivydkO/UhuJ X-Bogosity: Ham, tests=bogofilter, spamicity=0.103888, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Jan 08, 2025 at 06:58:00PM +0000, Lorenzo Stoakes wrote: > On Tue, Jan 07, 2025 at 10:48:02AM -0800, Isaac J. Manjarres wrote: > > diff --git a/mm/memfd.c b/mm/memfd.c > > index a9430090bb20..babf6433cf7b 100644 > > --- a/mm/memfd.c > > +++ b/mm/memfd.c > > @@ -394,26 +394,18 @@ static char *memfd_create_name(const char __user *uname) > > char *name; > > long len; > > > > - /* length includes terminating zero */ > > - len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1); > > - if (len <= 0) > > - return ERR_PTR(-EFAULT); > > - if (len > MFD_NAME_MAX_LEN + 1) > > - return ERR_PTR(-EINVAL); > > See below, but I think we should reinstate this. > > > - > > - name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL); > > + name = kmalloc(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1, GFP_KERNEL); > > This seems redundant as: > > #define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN) > > So MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1 > == MFD_NAME_PREFIX_LEN + NAME_MAX - MFD_NAME_PREFIX_LEN + 1 > == NAME_MAX + 1 > > So this should probably just be NAME_MAX + 1. > Thanks, that makes sense to me! I'll update it to NAME_MAX + 1 in v3 of the series. > > + len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1); > > This is sort of nitty, and actually optional honestly, but personally I really > find it a lot clearer to do: > > &name[MFD_NAME_PREFIX_LEN] > > Here, rather than pointer arithmetic, as it then clearly shows the offset. > That's reasonable; I'll make that change as well. > > goto err_name; > > - } > > - > > - /* terminating-zero may have changed after strnlen_user() returned */ > > - if (name[len + MFD_NAME_PREFIX_LEN - 1]) { > > - error = -EFAULT; > > + } else if (len > MFD_NAME_MAX_LEN) { > > + error = -EINVAL; > > I don't think this can ever happen? It just truncates, looking at the code > for strncpy_from_user(). > I double checked, and this case is possible. The maximum we allow to strncpy_from_user() to read is MFD_NAME_MAX_LEN + 1 via the count argument, so that includes the NULL terminator in the userspace buffer. strncpy_from_user() then returns the length of the string without the NULL terminator. The check is for just MFD_NAME_MAX_LEN, so this is meant to catch the case where the string, not including the NULL terminator, is greater than MFD_NAME_MAX_LEN, which is invalid, as well as the case where the string becomes malformed/corrupted mid-copy. Therefore, I think the cases that were caught before are still caught and handled in the same way. Is there something I'm missing? Thanks, Isaac