From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f170.google.com (mail-pl1-f170.google.com [209.85.214.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A89DA282FA for ; Thu, 9 Jan 2025 02:15:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736388943; cv=none; b=tK8LPaCNDX0Bg0sfjdzOYoEQ7aVdLXoSLYkg+/xBxW2XmklqD8xX+qxeWumubvW+loXWWIFzY4dc22xD1Imd4/L/VLlVWzMGL8ifuuJh0xwUs2EVRBvsD6OTgjtaADgzBT0PneGnza1V2xXtDoHqVRZk8vTunGh04pyR9FHknDw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736388943; c=relaxed/simple; bh=ahs5gfQJR9RSmnEza2vI4QLFJyKmZ2X2FJ15L5s2pOg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=VNGxlAWHmkdpPMmBtONp/LzQlOQtALFPTjQ7Pp5f9dd8HtupWk9k6ktkrLbbFwgV/HflmlEzrnl4bbCIKdSIewScYBfABjtYkbBZJLuNHCVM3Xk4Trwxse/npWXGJQfzodYeE0jLVZ2UMWTNjAthZCgUcz1WiH35fERUmlc32M8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=vko2fkr1; arc=none smtp.client-ip=209.85.214.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="vko2fkr1" Received: by mail-pl1-f170.google.com with SMTP id d9443c01a7336-219f6ca9a81so37185ad.1 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=vger.kernel.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=vko2fkr1gwVCxpmeDmrU/SvE3H2A6BJovqnfxQYSeOb11yzfK/M+eEOD+wagf82nJj Ys9ckzf6VBOT3BZyxpxuvBNOVTFHaSHd+Z70gzebhXrkkE1dVth6uitxgeUTV1TXClbb XACf/iui7fgU3fKEzeFGJafG2RxRfnFF6W+NeQLnrfG+sd1qUKkQe0rsnkFyEZAy8uIy GXOTUqeMFgpVGrfK3dKBEELHcMitUsECUJqE3/JkC7CHX/WhUFquYdI+uOEibbyqTJaL tpT//ckuvjCK7ojvpIxks6U8gfzUUHxS72HihBfCypWytNlmtqAkWtyo6K6Fb4po2pKp AEog== 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=Yi7NcVfqlQ/YaEr958vUgKWCG4oSBthtv5JSfFr6qnAyVQ7eh3uaIvwOCPN5E5Vwrb 0tEu2Wd4BlSnxBtzKX4JtpM75BiFxB/1RhNfUtDW3MVNzdu8bQETemOPy2qS7prPThJE 6gSa58/hdnsitBAabPbFWGcr152C8jdgD58A4QAVi06A7kXA+BP85NP6vv5ouRHHxOw3 xEv6jfP9yWZQJ9ppZSlr4MmXEwdiTFgkEnTwU8syk4teQfxybonTJeas92JQpFbqKp5x VEYpnt0uWw4lgXB7+eBNlnlyVuYF0Q0kYZ/Vehh4P+5Bt+TV+uqM7OLlBjZDqEr1G424 +bPg== X-Forwarded-Encrypted: i=1; AJvYcCVJpgu5C+H8L7frmxA2ZlMlZ2gVw7TV0u6eGBstuILlJKT4nxbc7NReBPNd4biyBR2+9Ls4fEVa1Vu1PEc=@vger.kernel.org X-Gm-Message-State: AOJu0Yww5m8JrIfoFGzV9c1PzqmA3vRHJoR2qZCv9j3ZvyvUy1IN102Y GvpDCW1UPFvSDiKfjl3m9YQ9XZbQla7H0xtS71vfJSEgRs13t/f+Wt066jiokw== X-Gm-Gg: ASbGnctUoDueBtDJq1R+OrOszIK48FQuBHg7KVwTEXtRlNGyfr+s4Xp6zvVMCB6zD3R YOv1MInWWYQTAaAdvWWl4hIgoEDsH5iGaudhd+q3f6mp3sgMDy6RdnXsrp0zPXiEdXBF8rjVYuw nGh/O47MDT/Bg0XTwoM/VQSDQwetDQ2V0vFIcPdaZnVWt55DnWlZDKDHFUpV4zZ1ODYpJh5PE2C gKC0x7x1D/IYalMEBYVQPJfR5D2b4iYMQG+N2NiLKvJ+94RePLsWkv92Q== 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> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4291d9f0-4483-40e5-a54b-d006eb52c8cb@lucifer.local> 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