From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (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 4C3B433031C for ; Thu, 11 Jun 2026 03:50:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781149839; cv=none; b=KQpigXGFS1JBFRVU+iUruViTBDwHxweT0e9I6KVELeSJKrU83udeagSTVaGsAQdE7XxwLFe0tBcMj0HxdELi8lWZPlb6Bytr56Ddvr6cSpJaGW6set7ptwMPBFftTHlXQCHSAnZ9fdYpP1FMp9qUFioKFTmytjIkLfdMkoM0Tlc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781149839; c=relaxed/simple; bh=gz/msjdtOunMEex2BqTVGDc/ZG31euGpU4mAhP/QZRc=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=Y7367gReJGgxBhP2w/4OCMkPzO1FELPojiMiwMeP+fN2qXnlP71Xg5EcBBxQivKvfKxD6jFQBz/zonLqybCxb4XaA8wmqmYPqe6fuOCdd/3xHQ7KTTcm57Xk4gzVrWV9FfhwGwg3BShLX6sj9oKc0qesW0P+TqAbyXtnjXuuh2E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=dubeyko.com; spf=pass smtp.mailfrom=dubeyko.com; dkim=pass (2048-bit key) header.d=dubeyko-com.20251104.gappssmtp.com header.i=@dubeyko-com.20251104.gappssmtp.com header.b=Eg91DVsc; arc=none smtp.client-ip=209.85.167.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=dubeyko.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=dubeyko.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=dubeyko-com.20251104.gappssmtp.com header.i=@dubeyko-com.20251104.gappssmtp.com header.b="Eg91DVsc" Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-5aa68d65d24so7953928e87.1 for ; Wed, 10 Jun 2026 20:50:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dubeyko-com.20251104.gappssmtp.com; s=20251104; t=1781149834; x=1781754634; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=FVrzkqwtVWnUjAk0pQWrCKuHIZWdAIMxGLk/tntdOl4=; b=Eg91DVscJNYnZU9JOPFYnyXIxVJc52iQBBSVwinl7VUijWvWXslkckyw5+vOoB+j79 P3eGDZizn4/ZiPFZ5M1GgvLKJNJxeuQtDDRhEYB/U9MqpdpVmThzEEg5i23RGJdav3/u CIGo4INay1vUzQuTxbLObLxF8ohOQtt1hoNkmgNN/EGq0qEGJhENJ52Y5XcMc/Jn/9nr eqlT7t201aI0mqrDC20H1cqrBk8fVDtkBAOncqs7u2L25J7+doCigq9iwm1F7LeeykT2 rNBOyOA/mZZQxIsOg/Oj2YonN32T5UyHjP74FCGM0VGcnrUuLaofM1splyLvLD7GYp+3 zcJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781149834; x=1781754634; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=FVrzkqwtVWnUjAk0pQWrCKuHIZWdAIMxGLk/tntdOl4=; b=NKNwlz7zfxwqICledmqqOAGFaX4wwQ9axllH3qP6de11Q6MOVhDoRV+uHaRYXNopzi trG79nl0JMIZ2+z+YGFQuxoSaSwZ3sEaKrSORCP+UODyrf6cY9pn0pMIp0YHMvA4YpU4 J8UZoQkhDOSHAKHi1cTV/8LPUagCWsZakaCG+Nhm8t+mxkA6E63pGDvOjl1vqJsS1VCr dWp2JZZ99dqyjd6+svmk+DCHU0Bh7J3JwfD5d4qqJ+Muk87263aiVhAuwtbWFp0r2OlJ zdrsNegocGPVHST5pKxzIrT6d2OoMb3QHmr4V5nmIqnqzmvm/S14ZN09af40x6ooyWnu uJcQ== X-Forwarded-Encrypted: i=1; AFNElJ9J0QOWgWr4h7+P3BgwN+k+PqxJx4W5Tp2omsyQF3OXdlo5TMivetX9+x0Q9IEnuYKIcXxSYbmkNQFoFLs=@vger.kernel.org X-Gm-Message-State: AOJu0YyPYKFzyCybeUuXZZn27e1AJwzhcFy3ZxD6CyOAaFH4Q/ySbam5 Wk/CbaSW07kD3PyC7P4qKI8Y/amqR2GgA8IIynkuBhjT1TvZtclnuNKf9f2WcDKUNMs= X-Gm-Gg: Acq92OGxFielBPqIorDlj4b1kZziBJqprbx9N9GM/pyaKFV+U9cQA1fVo2DDX409lSY o1ryopYJ9/rkHVr0pKFmapypNgTyuV/8otyl6LufFXcC3LeBzdg2ZnuTlFNOR2nxxA3q75BhHHd gyEvNDRsVgjrCcoSy8F0WSdmIft21kUSik3pggxCiPY6RaWFR9AF6zu2o3r1SUJjA1TQEVx0Qk3 OO8XPqE646EZ6VDhVARNFMhtLI6Zz/VMVO6eQji5IP+A49UgD/37ucBzyjfwytocz3XA4AkfPpA SGefQeqJwDJGtkTbY6KfAQZul6Qb73j73Y+SFS58TeknjxtbE2/f/iaObmKAOXXNvfKqcLQyrjY yVRQmJxsXiYpdArCNiniU8EDWStd59ko0DzWo5DasPYGLUkE3JdT+/IwnLIjqqBR0IE5y8H8Hm8 XfsYiqdvvO3XyxJkSCB1Yinnmzfq7QvdKTU9O1nartdpmZCcsqJoAdlOAGn7KJbLO2Mtl4fDLFe cSWX3sOOjf8dMD2oxw/MAf7Jm0D3PD2iST6YTUhJidSwnrvHr8XAe9N4BLg1Nt7A7g= X-Received: by 2002:ac2:51cd:0:b0:5aa:77b2:fec8 with SMTP id 2adb3069b0e04-5ad27fb70bfmr251340e87.27.1781149834211; Wed, 10 Jun 2026 20:50:34 -0700 (PDT) Received: from [192.168.1.246] (broadband-95-84-186-252.ip.moscow.rt.ru. [95.84.186.252]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-5aa7b8fc8d9sm5921278e87.28.2026.06.10.20.50.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Jun 2026 20:50:33 -0700 (PDT) Message-ID: <2543b22369ec19dae397963fbdf2e7181c7419a8.camel@dubeyko.com> Subject: Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name From: Viacheslav Dubeyko To: david.laight.linux@gmail.com, Kees Cook , linux-hardening@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Arnd Bergmann , John Paul Adrian Glaubitz , Yangtao Li Date: Wed, 10 Jun 2026 20:50:33 -0700 In-Reply-To: <20260608095523.2606-39-david.laight.linux@gmail.com> References: <20260608095523.2606-39-david.laight.linux@gmail.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.1 (by Flathub.org) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote: > From: David Laight >=20 > xattr_name is kmalloc()ed at the (assumed) maximal size and then the > prefix > and name concatenated together. > Use memcpy() for the prefix - its length is passed and strscpy() for > the > name to ensure it really doesnt overflow. >=20 > Prior to bf29e886b242c the buffers were smaller and on-stack. > (But I cant see the copy in the old code.) > I am also not sure why the buffer isnt created "just long enough". >=20 > Signed-off-by: David Laight > --- > This is one of a group of patches that remove potentially unbounded > strcpy() calls. >=20 > They are mostly replaced by strscpy() or, when strlen() has just been > called, with memcpy() (usually including the '\0'). >=20 > Calls with copy string literals into arrays are left unchanged. > They are safe and easily detected as such. >=20 > The changes were made by getting the compiler to detect the calls and > then fixing the code by hand. >=20 > Note that all the changes are only compile tested. >=20 > Some Makefiles were changed to allow files to contain strcpy(). > As well as 'difficult to fix' files, this included 'show' functions > as they really need to use sysfs_emit() or seq_printf(). >=20 > All the patches are being sent individually to avoid very long cc > lists. > Apologies for the terse commit messages and likely unexpected tags. > (There are about 100 patches in total.) >=20 > =C2=A0fs/hfsplus/xattr.c | 12 ++++++------ > =C2=A01 file changed, 6 insertions(+), 6 deletions(-) >=20 > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c > index 452a1f9becb2..0b3dd48c28c9 100644 > --- a/fs/hfsplus/xattr.c > +++ b/fs/hfsplus/xattr.c > @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const > char *name, > =C2=A0 xattr_name =3D kmalloc(xattr_name_len, GFP_KERNEL); > =C2=A0 if (!xattr_name) > =C2=A0 return -ENOMEM; > - strcpy(xattr_name, prefix); > - strcpy(xattr_name + prefixlen, name); > + memcpy(xattr_name, prefix, prefixlen); What's the point to mix memcpy and str*() family of methods? What's wrong with str*() method here? Otherwise, if it is wrong to use str*() family of methods, then why is it correct to use for second operation? > + strscpy(xattr_name + prefixlen, name, xattr_name_len - > prefixlen); Why strscpy() is better than strncpy()? What is the main argument here? > =C2=A0 res =3D __hfsplus_setxattr(inode, xattr_name, value, size, > flags); > =C2=A0 kfree(xattr_name); > =C2=A0 > @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode, > const char *name, > =C2=A0 void *value, size_t size, > =C2=A0 const char *prefix, size_t prefixlen) > =C2=A0{ > + size_t xattr_name_len =3D NLS_MAX_CHARSET_SIZE * > HFSPLUS_ATTR_MAX_STRLEN + 1; Frankly speaking, it looks like a constant that should be declared in hfs_common.h. Even if we would like to declare it here, then it should be const size_t, from my point of view. > =C2=A0 int res; > =C2=A0 char *xattr_name; > =C2=A0 > @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode, > const char *name, > =C2=A0 inode->i_ino, name ? name : NULL, > =C2=A0 prefix ? prefix : NULL); > =C2=A0 > - xattr_name =3D kmalloc(NLS_MAX_CHARSET_SIZE * > HFSPLUS_ATTR_MAX_STRLEN + 1, > - =C2=A0=C2=A0=C2=A0=C2=A0 GFP_KERNEL); > + xattr_name =3D kmalloc(xattr_name_len, GFP_KERNEL); Finally, I think kzalloc() should be much better for both cases. Thanks, Slava. > =C2=A0 if (!xattr_name) > =C2=A0 return -ENOMEM; > =C2=A0 > - strcpy(xattr_name, prefix); > - strcpy(xattr_name + prefixlen, name); > + memcpy(xattr_name, prefix, prefixlen); > + strscpy(xattr_name + prefixlen, name, xattr_name_len - > prefixlen); > =C2=A0 > =C2=A0 res =3D __hfsplus_getxattr(inode, xattr_name, value, size); > =C2=A0 kfree(xattr_name);