From patchwork Thu Mar 21 23:46:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Stitt X-Patchwork-Id: 13599468 Received: from mail-yb1-f202.google.com (mail-yb1-f202.google.com [209.85.219.202]) (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 5D37EF516 for ; Thu, 21 Mar 2024 23:46:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.219.202 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711064794; cv=none; b=W6bnKdUdVKu5G3VHX658/araEkTECLbymRk8NEqM2OaqXBCgYaxCdJ2aPni4T7WCxFCntSCzIg9EjEHL3SlUUcQOVma394G4CX2xNJiTR+wum2X8jNgH+XkBBNFCuQbKLtsCY7yCWeG5pE1wILGV2txRsPIP1yQ2etVxZlqq0K4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711064794; c=relaxed/simple; bh=GBp117Rw9ou09bC9vlczg3WmwjqjNQ4BV42hmHgOAGM=; h=Date:Mime-Version:Message-ID:Subject:From:To:Cc:Content-Type; b=Eud1qnJRLCPs4pUJvtotLk1FjWQBLe0coattIhbf9nYbHnXddd3bqOpTA7TLo1hJ/fdKYQRv6PpOaALmk3ZR1YUT84d1bg0x3orZUS9kkRbbHKw6Pi9B1YmeGe4NuHHOnudBM6o/P630FnWV6eIY6AVeNmJuBetsE7qKiVgqap4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--justinstitt.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=VsCJse60; arc=none smtp.client-ip=209.85.219.202 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=flex--justinstitt.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="VsCJse60" Received: by mail-yb1-f202.google.com with SMTP id 3f1490d57ef6-dcd1779adbeso2408585276.3 for ; Thu, 21 Mar 2024 16:46:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1711064791; x=1711669591; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=TKcHHCSWCqgul2AOMbuKphpbCV9drtQafSH0+0DTZms=; b=VsCJse60j86WLf2PNMFujVaQX/q9AyhsHXmv/C09+wPlP+I0eZpkWyVD2nuHIr3e2w NsGGQJV5cM/vpLFlHwltfelhumVPrZR13P5ANwKcFg82SeZY9ULvH/FytwRXQTwCrYOI q8sitGGwDRXrgrziLBI6tJGwsJe3E13S4xFCW1BPRfOrawWEkh95DkZ1fmFUelM0zJj9 g9fjDuAjILXtG7hGSbDt0wodKzHspkM/8kb3Ic/wV0fvs4U5UozufWb/ndF9eyK0GbY8 gckRVGUM09dqaPOSPeL9t1MJZ+Nu093QYtukD/aDnHaX+VfZjnKRwbzFKF3n5ZHfoj7d CxNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711064791; x=1711669591; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=TKcHHCSWCqgul2AOMbuKphpbCV9drtQafSH0+0DTZms=; b=XRHxfRyyOYr+bIFAyWthfwne9y386k/omuNmNT/O+b/Yo9S8O3qLdBs6Ue4PPSkf1a NUL4vTDwsIQAXEpxJh0FSYiUPhLEHoDcze6Nqckm5bjHcBFV1pbyXA+NkqdtevR2l3No sny5DbEGkWywhAcaTl5kPBMiLpz+BDNgh0W4ilYFeUhdcgzXryvqiTkHG8x0uYPpnJjN SF47e12wb73NRkWVlTqFClN4yb1rs/sCnb7Gvfn3F9eh/B2YAQ8uR0CMV7ok/evA6eYe 6revCuHoaIszYBro7qwOITTZw8WXl6RejpWHcXtbHTUN4sAKpb8Hwp1iVZsBMSXtUJwo oIpg== X-Forwarded-Encrypted: i=1; AJvYcCUHsrbwr3fG1UUGDcKPZcq+21C8xHLJm28nwaCafrL8aCumiVGara2emRFd34fOgZKOFS6Vvur5JbjiWn1BoXqrW3THn97NO8q//TqAVkRG X-Gm-Message-State: AOJu0Yx7smt2T6MQPL5OZXsMgyLJZyw1yTOCCyAMqKO+PnnmmG3Thexi rwXRj0D6MjVO8aPzSVtURiC5f7usctV5/FIiIXq3oN0AAPfViyTlkT29ozTkglXAOwtPCVrcdMx FNQc3+WMDqiW/Za5GH/Gu/A== X-Google-Smtp-Source: AGHT+IGslFJ7I58NprLlYtxvFu8RungPyoqKiB8iuMyQ7S0Xf45CcMjYFnGLqEJkvq/9SAAdc0Ta5veeJ5B3mcEUmw== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a05:6902:100a:b0:dc6:ff2b:7e1b with SMTP id w10-20020a056902100a00b00dc6ff2b7e1bmr196400ybt.4.1711064791522; Thu, 21 Mar 2024 16:46:31 -0700 (PDT) Date: Thu, 21 Mar 2024 23:46:27 +0000 Precedence: bulk X-Mailing-List: linux-hardening@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIANLG/GUC/x3MwQqDMAwA0F+RnA1oLQr+ytjB1WQGpJakikP8d 8uO7/IuMFIhg7G6QOkQky0WtHUFYZnil1DmYnCN803nWrSsMaQfsuHCltbd8JxyVgzo6cPUD+x DP0MJkhLL+c9f7/t+AA3Ex21sAAAA X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1711064790; l=3560; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=GBp117Rw9ou09bC9vlczg3WmwjqjNQ4BV42hmHgOAGM=; b=7eliCTZlOhrbirCoWB7C+nw7yQJFi+JZPUgfpGG0LQzr9Zvxig7qUqczMOmzK471p6Oo4nxWE EZp5nx2evR3AhFnqMtbF8E/P5IbKnWIgUu8C3qx4/qaT1i9Z166dvrZ X-Mailer: b4 0.12.3 Message-ID: <20240321-strncpy-fs-hfsplus-xattr-c-v1-1-0c6385a10251@google.com> Subject: [PATCH] hfsplus: refactor copy_name to not use strncpy From: Justin Stitt To: Kees Cook Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Justin Stitt strncpy() is deprecated with NUL-terminated destination strings [1]. The copy_name() method does a lot of manual buffer manipulation to eventually arrive with its desired string. If we don't know the namespace this attr has or belongs to we want to prepend "osx." to our final string. Following this, we're copying xattr_name and doing a bizarre manual NUL-byte assignment with a memset where n=1. Really, we can use some more obvious string APIs to acomplish this, improving readability and security. Following the same control flow as before: if we don't know the namespace let's use scnprintf() to form our prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use strscpy() to return the number of bytes copied into our buffer. Note that strscpy() _can_ return -E2BIG but this is already handled by all callsites: In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is already type ssize_t so we can change the return type of copy_name() to match (understanding that scnprintf()'s return type is different yet fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c is well-equipped to handle a potential -E2BIG return result from vfs_listxattr(): | ssize_t error; ... | error = vfs_listxattr(d, klist, size); | if (error > 0) { | if (size && copy_to_user(list, klist, error)) | error = -EFAULT; | } else if (error == -ERANGE && size >= XATTR_LIST_MAX) { | /* The file system tried to returned a list bigger | than XATTR_LIST_MAX bytes. Not possible. */ | error = -E2BIG; | } ... our error can potentially already be -E2BIG, skipping this else-if and ending up at the same state as other errors. This whole copy_name() function could really be a one-line with some ternary statements embedded into a scnprintf() arg-list but I've opted to maintain some semblance of readability. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt --- --- fs/hfsplus/xattr.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) --- base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb change-id: 20240321-strncpy-fs-hfsplus-xattr-c-4ebfe67f4c6d Best regards, -- Justin Stitt diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c index 9c9ff6b8c6f7..00351f566e9f 100644 --- a/fs/hfsplus/xattr.c +++ b/fs/hfsplus/xattr.c @@ -400,22 +400,13 @@ static int name_len(const char *xattr_name, int xattr_name_len) return len; } -static int copy_name(char *buffer, const char *xattr_name, int name_len) +static ssize_t copy_name(char *buffer, const char *xattr_name, int name_len) { - int len = name_len; - int offset = 0; - - if (!is_known_namespace(xattr_name)) { - memcpy(buffer, XATTR_MAC_OSX_PREFIX, XATTR_MAC_OSX_PREFIX_LEN); - offset += XATTR_MAC_OSX_PREFIX_LEN; - len += XATTR_MAC_OSX_PREFIX_LEN; - } - - strncpy(buffer + offset, xattr_name, name_len); - memset(buffer + offset + name_len, 0, 1); - len += 1; + if (!is_known_namespace(xattr_name)) + return scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN, + "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name); - return len; + return strscpy(buffer, xattr_name, name_len + 1); } int hfsplus_setxattr(struct inode *inode, const char *name,