[PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[integral]

Use safe string copying macros (strscpy & strscpy_pad) to make string
copying simpler & safer.

Signed-off-by: integral@archlinuxcn.org <integral@archlinuxcn.org>
---
 fs/bcachefs/dirent.c      | 10 +++-------
 fs/bcachefs/disk_groups.c |  5 ++---
 fs/bcachefs/fs-ioctl.c    | 17 ++++++++---------
 fs/bcachefs/fs.c          |  6 +-----
 4 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/fs/bcachefs/dirent.c b/fs/bcachefs/dirent.c
index 4c22f78b0484..466fe5765739 100644
--- a/fs/bcachefs/dirent.c
+++ b/fs/bcachefs/dirent.c
@@ -191,13 +191,9 @@ static struct bkey_i_dirent *dirent_create_key(struct btree_trans *trans,
 
 	dirent->v.d_type = type;
 
-	memcpy(dirent->v.d_name, name->name, name->len);
-	memset(dirent->v.d_name + name->len, 0,
-	       bkey_val_bytes(&dirent->k) -
-	       offsetof(struct bch_dirent, d_name) -
-	       name->len);
-
-	EBUG_ON(bch2_dirent_name_bytes(dirent_i_to_s_c(dirent)) != name->len);
+	EBUG_ON(strscpy_pad(dirent->v.d_name, name->name,
+			    bkey_val_bytes(&dirent->k) - offsetof(struct bch_dirent, d_name))
+		!= name->len);
 
 	return dirent;
 }
diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
index 5df8de0b8c02..c2fc575e38d1 100644
--- a/fs/bcachefs/disk_groups.c
+++ b/fs/bcachefs/disk_groups.c
@@ -319,9 +319,8 @@ static int __bch2_disk_group_add(struct bch_sb_handle *sb, unsigned parent,
 
 	g = &groups->entries[i];
 
-	memcpy(g->label, name, namelen);
-	if (namelen < sizeof(g->label))
-		g->label[namelen] = '\0';
+	strscpy(g->label, name, namelen);
+
 	SET_BCH_GROUP_DELETED(g, 0);
 	SET_BCH_GROUP_PARENT(g, parent);
 	SET_BCH_GROUP_DATA_ALLOWED(g, ~0);
diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
index 15725b4ce393..4233707bec9d 100644
--- a/fs/bcachefs/fs-ioctl.c
+++ b/fs/bcachefs/fs-ioctl.c
@@ -279,26 +279,25 @@ static int bch2_ioc_getversion(struct bch_inode_info *inode, u32 __user *arg)
 
 static int bch2_ioc_getlabel(struct bch_fs *c, char __user *user_label)
 {
-	int ret;
-	size_t len;
 	char label[BCH_SB_LABEL_SIZE];
+	size_t len;
 
 	BUILD_BUG_ON(BCH_SB_LABEL_SIZE >= FSLABEL_MAX);
 
 	mutex_lock(&c->sb_lock);
-	memcpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
+	int ret = strscpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
 	mutex_unlock(&c->sb_lock);
 
-	len = strnlen(label, BCH_SB_LABEL_SIZE);
-	if (len == BCH_SB_LABEL_SIZE) {
+	if (ret == -E2BIG) {
+		len = BCH_SB_LABEL_SIZE - 1;
 		bch_warn(c,
 			"label is too long, return the first %zu bytes",
-			--len);
+			len);
+	} else {
+		len = ret;
 	}
 
-	ret = copy_to_user(user_label, label, len);
-
-	return ret ? -EFAULT : 0;
+	return copy_to_user(user_label, label, len) ? -EFAULT : 0;
 }
 
 static int bch2_ioc_setlabel(struct bch_fs *c,
diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 91fce04272a1..be86a36e8cfe 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1641,7 +1641,6 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	subvol_inum target;
 	u32 snapshot;
 	struct qstr dirent_name;
-	unsigned name_len = 0;
 	int ret;
 
 	if (!S_ISDIR(dir->v.i_mode))
@@ -1717,10 +1716,7 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	goto err;
 found:
 	dirent_name = bch2_dirent_get_name(d);
-
-	name_len = min_t(unsigned, dirent_name.len, NAME_MAX);
-	memcpy(name, dirent_name.name, name_len);
-	name[name_len] = '\0';
+	strscpy(name, dirent_name.name, NAME_MAX);
 err:
 	if (bch2_err_matches(ret, BCH_ERR_transaction_restart))
 		goto retry;
-- 
2.47.0

[PATCH resend] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[Integral]

Use safe string copying macros (strscpy & strscpy_pad) to make string
copying simpler & safer.
---
 fs/bcachefs/dirent.c      | 10 +++-------
 fs/bcachefs/disk_groups.c |  5 ++---
 fs/bcachefs/fs-ioctl.c    | 17 ++++++++---------
 fs/bcachefs/fs.c          |  6 +-----
 4 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/fs/bcachefs/dirent.c b/fs/bcachefs/dirent.c
index 600eee936f13..8bc6696115c0 100644
--- a/fs/bcachefs/dirent.c
+++ b/fs/bcachefs/dirent.c
@@ -191,13 +191,9 @@ static struct bkey_i_dirent *dirent_create_key(struct btree_trans *trans,
 
 	dirent->v.d_type = type;
 
-	memcpy(dirent->v.d_name, name->name, name->len);
-	memset(dirent->v.d_name + name->len, 0,
-	       bkey_val_bytes(&dirent->k) -
-	       offsetof(struct bch_dirent, d_name) -
-	       name->len);
-
-	EBUG_ON(bch2_dirent_name_bytes(dirent_i_to_s_c(dirent)) != name->len);
+	EBUG_ON(strscpy_pad(dirent->v.d_name, name->name,
+			    bkey_val_bytes(&dirent->k) - offsetof(struct bch_dirent, d_name))
+		!= name->len);
 
 	return dirent;
 }
diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
index 5df8de0b8c02..c2fc575e38d1 100644
--- a/fs/bcachefs/disk_groups.c
+++ b/fs/bcachefs/disk_groups.c
@@ -319,9 +319,8 @@ static int __bch2_disk_group_add(struct bch_sb_handle *sb, unsigned parent,
 
 	g = &groups->entries[i];
 
-	memcpy(g->label, name, namelen);
-	if (namelen < sizeof(g->label))
-		g->label[namelen] = '\0';
+	strscpy(g->label, name, namelen);
+
 	SET_BCH_GROUP_DELETED(g, 0);
 	SET_BCH_GROUP_PARENT(g, parent);
 	SET_BCH_GROUP_DATA_ALLOWED(g, ~0);
diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
index 15725b4ce393..4233707bec9d 100644
--- a/fs/bcachefs/fs-ioctl.c
+++ b/fs/bcachefs/fs-ioctl.c
@@ -279,26 +279,25 @@ static int bch2_ioc_getversion(struct bch_inode_info *inode, u32 __user *arg)
 
 static int bch2_ioc_getlabel(struct bch_fs *c, char __user *user_label)
 {
-	int ret;
-	size_t len;
 	char label[BCH_SB_LABEL_SIZE];
+	size_t len;
 
 	BUILD_BUG_ON(BCH_SB_LABEL_SIZE >= FSLABEL_MAX);
 
 	mutex_lock(&c->sb_lock);
-	memcpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
+	int ret = strscpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
 	mutex_unlock(&c->sb_lock);
 
-	len = strnlen(label, BCH_SB_LABEL_SIZE);
-	if (len == BCH_SB_LABEL_SIZE) {
+	if (ret == -E2BIG) {
+		len = BCH_SB_LABEL_SIZE - 1;
 		bch_warn(c,
 			"label is too long, return the first %zu bytes",
-			--len);
+			len);
+	} else {
+		len = ret;
 	}
 
-	ret = copy_to_user(user_label, label, len);
-
-	return ret ? -EFAULT : 0;
+	return copy_to_user(user_label, label, len) ? -EFAULT : 0;
 }
 
 static int bch2_ioc_setlabel(struct bch_fs *c,
diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 3f83f131d0e8..b553dae38e80 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1656,7 +1656,6 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	subvol_inum target;
 	u32 snapshot;
 	struct qstr dirent_name;
-	unsigned name_len = 0;
 	int ret;
 
 	if (!S_ISDIR(dir->v.i_mode))
@@ -1732,10 +1731,7 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	goto err;
 found:
 	dirent_name = bch2_dirent_get_name(d);
-
-	name_len = min_t(unsigned, dirent_name.len, NAME_MAX);
-	memcpy(name, dirent_name.name, name_len);
-	name[name_len] = '\0';
+	strscpy(name, dirent_name.name, NAME_MAX);
 err:
 	if (bch2_err_matches(ret, BCH_ERR_transaction_restart))
 		goto retry;
-- 
2.47.1

[PATCH resend v2] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[Integral]

Use safe string copying macros (strscpy & strscpy_pad) to make string
copying simpler & safer.

Signed-off-by: Integral <integral@archlinuxcn.org>
---
 fs/bcachefs/dirent.c      | 10 +++-------
 fs/bcachefs/disk_groups.c |  5 ++---
 fs/bcachefs/fs-ioctl.c    | 17 ++++++++---------
 fs/bcachefs/fs.c          |  6 +-----
 4 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/fs/bcachefs/dirent.c b/fs/bcachefs/dirent.c
index 600eee936f13..8bc6696115c0 100644
--- a/fs/bcachefs/dirent.c
+++ b/fs/bcachefs/dirent.c
@@ -191,13 +191,9 @@ static struct bkey_i_dirent *dirent_create_key(struct btree_trans *trans,
 
 	dirent->v.d_type = type;
 
-	memcpy(dirent->v.d_name, name->name, name->len);
-	memset(dirent->v.d_name + name->len, 0,
-	       bkey_val_bytes(&dirent->k) -
-	       offsetof(struct bch_dirent, d_name) -
-	       name->len);
-
-	EBUG_ON(bch2_dirent_name_bytes(dirent_i_to_s_c(dirent)) != name->len);
+	EBUG_ON(strscpy_pad(dirent->v.d_name, name->name,
+			    bkey_val_bytes(&dirent->k) - offsetof(struct bch_dirent, d_name))
+		!= name->len);
 
 	return dirent;
 }
diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
index 5df8de0b8c02..c2fc575e38d1 100644
--- a/fs/bcachefs/disk_groups.c
+++ b/fs/bcachefs/disk_groups.c
@@ -319,9 +319,8 @@ static int __bch2_disk_group_add(struct bch_sb_handle *sb, unsigned parent,
 
 	g = &groups->entries[i];
 
-	memcpy(g->label, name, namelen);
-	if (namelen < sizeof(g->label))
-		g->label[namelen] = '\0';
+	strscpy(g->label, name, namelen);
+
 	SET_BCH_GROUP_DELETED(g, 0);
 	SET_BCH_GROUP_PARENT(g, parent);
 	SET_BCH_GROUP_DATA_ALLOWED(g, ~0);
diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
index 15725b4ce393..4233707bec9d 100644
--- a/fs/bcachefs/fs-ioctl.c
+++ b/fs/bcachefs/fs-ioctl.c
@@ -279,26 +279,25 @@ static int bch2_ioc_getversion(struct bch_inode_info *inode, u32 __user *arg)
 
 static int bch2_ioc_getlabel(struct bch_fs *c, char __user *user_label)
 {
-	int ret;
-	size_t len;
 	char label[BCH_SB_LABEL_SIZE];
+	size_t len;
 
 	BUILD_BUG_ON(BCH_SB_LABEL_SIZE >= FSLABEL_MAX);
 
 	mutex_lock(&c->sb_lock);
-	memcpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
+	int ret = strscpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
 	mutex_unlock(&c->sb_lock);
 
-	len = strnlen(label, BCH_SB_LABEL_SIZE);
-	if (len == BCH_SB_LABEL_SIZE) {
+	if (ret == -E2BIG) {
+		len = BCH_SB_LABEL_SIZE - 1;
 		bch_warn(c,
 			"label is too long, return the first %zu bytes",
-			--len);
+			len);
+	} else {
+		len = ret;
 	}
 
-	ret = copy_to_user(user_label, label, len);
-
-	return ret ? -EFAULT : 0;
+	return copy_to_user(user_label, label, len) ? -EFAULT : 0;
 }
 
 static int bch2_ioc_setlabel(struct bch_fs *c,
diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 3f83f131d0e8..b553dae38e80 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1656,7 +1656,6 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	subvol_inum target;
 	u32 snapshot;
 	struct qstr dirent_name;
-	unsigned name_len = 0;
 	int ret;
 
 	if (!S_ISDIR(dir->v.i_mode))
@@ -1732,10 +1731,7 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
 	goto err;
 found:
 	dirent_name = bch2_dirent_get_name(d);
-
-	name_len = min_t(unsigned, dirent_name.len, NAME_MAX);
-	memcpy(name, dirent_name.name, name_len);
-	name[name_len] = '\0';
+	strscpy(name, dirent_name.name, NAME_MAX);
 err:
 	if (bch2_err_matches(ret, BCH_ERR_transaction_restart))
 		goto retry;
-- 
2.47.1

Re: [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[Kent Overstreet]

On Sun, Nov 10, 2024 at 02:41:38PM +0800, integral@archlinuxcn.org wrote:
> Use safe string copying macros (strscpy & strscpy_pad) to make string
> copying simpler & safer.
> 
> Signed-off-by: integral@archlinuxcn.org <integral@archlinuxcn.org>

No!

You're introducing nuls where there previously weren't any, and since
there are strict checks on how many padding nuls are allowed this is
broken.

I already told you this didn't need to be done; please stop.

> ---
>  fs/bcachefs/dirent.c      | 10 +++-------
>  fs/bcachefs/disk_groups.c |  5 ++---
>  fs/bcachefs/fs-ioctl.c    | 17 ++++++++---------
>  fs/bcachefs/fs.c          |  6 +-----
>  4 files changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/bcachefs/dirent.c b/fs/bcachefs/dirent.c
> index 4c22f78b0484..466fe5765739 100644
> --- a/fs/bcachefs/dirent.c
> +++ b/fs/bcachefs/dirent.c
> @@ -191,13 +191,9 @@ static struct bkey_i_dirent *dirent_create_key(struct btree_trans *trans,
>  
>  	dirent->v.d_type = type;
>  
> -	memcpy(dirent->v.d_name, name->name, name->len);
> -	memset(dirent->v.d_name + name->len, 0,
> -	       bkey_val_bytes(&dirent->k) -
> -	       offsetof(struct bch_dirent, d_name) -
> -	       name->len);
> -
> -	EBUG_ON(bch2_dirent_name_bytes(dirent_i_to_s_c(dirent)) != name->len);
> +	EBUG_ON(strscpy_pad(dirent->v.d_name, name->name,
> +			    bkey_val_bytes(&dirent->k) - offsetof(struct bch_dirent, d_name))
> +		!= name->len);
>  
>  	return dirent;
>  }
> diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
> index 5df8de0b8c02..c2fc575e38d1 100644
> --- a/fs/bcachefs/disk_groups.c
> +++ b/fs/bcachefs/disk_groups.c
> @@ -319,9 +319,8 @@ static int __bch2_disk_group_add(struct bch_sb_handle *sb, unsigned parent,
>  
>  	g = &groups->entries[i];
>  
> -	memcpy(g->label, name, namelen);
> -	if (namelen < sizeof(g->label))
> -		g->label[namelen] = '\0';
> +	strscpy(g->label, name, namelen);
> +
>  	SET_BCH_GROUP_DELETED(g, 0);
>  	SET_BCH_GROUP_PARENT(g, parent);
>  	SET_BCH_GROUP_DATA_ALLOWED(g, ~0);
> diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> index 15725b4ce393..4233707bec9d 100644
> --- a/fs/bcachefs/fs-ioctl.c
> +++ b/fs/bcachefs/fs-ioctl.c
> @@ -279,26 +279,25 @@ static int bch2_ioc_getversion(struct bch_inode_info *inode, u32 __user *arg)
>  
>  static int bch2_ioc_getlabel(struct bch_fs *c, char __user *user_label)
>  {
> -	int ret;
> -	size_t len;
>  	char label[BCH_SB_LABEL_SIZE];
> +	size_t len;
>  
>  	BUILD_BUG_ON(BCH_SB_LABEL_SIZE >= FSLABEL_MAX);
>  
>  	mutex_lock(&c->sb_lock);
> -	memcpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
> +	int ret = strscpy(label, c->disk_sb.sb->label, BCH_SB_LABEL_SIZE);
>  	mutex_unlock(&c->sb_lock);
>  
> -	len = strnlen(label, BCH_SB_LABEL_SIZE);
> -	if (len == BCH_SB_LABEL_SIZE) {
> +	if (ret == -E2BIG) {
> +		len = BCH_SB_LABEL_SIZE - 1;
>  		bch_warn(c,
>  			"label is too long, return the first %zu bytes",
> -			--len);
> +			len);
> +	} else {
> +		len = ret;
>  	}
>  
> -	ret = copy_to_user(user_label, label, len);
> -
> -	return ret ? -EFAULT : 0;
> +	return copy_to_user(user_label, label, len) ? -EFAULT : 0;
>  }
>  
>  static int bch2_ioc_setlabel(struct bch_fs *c,
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index 91fce04272a1..be86a36e8cfe 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -1641,7 +1641,6 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
>  	subvol_inum target;
>  	u32 snapshot;
>  	struct qstr dirent_name;
> -	unsigned name_len = 0;
>  	int ret;
>  
>  	if (!S_ISDIR(dir->v.i_mode))
> @@ -1717,10 +1716,7 @@ static int bch2_get_name(struct dentry *parent, char *name, struct dentry *child
>  	goto err;
>  found:
>  	dirent_name = bch2_dirent_get_name(d);
> -
> -	name_len = min_t(unsigned, dirent_name.len, NAME_MAX);
> -	memcpy(name, dirent_name.name, name_len);
> -	name[name_len] = '\0';
> +	strscpy(name, dirent_name.name, NAME_MAX);
>  err:
>  	if (bch2_err_matches(ret, BCH_ERR_transaction_restart))
>  		goto retry;
> -- 
> 2.47.0
> 

Re: [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer

[Integral]

I apologize for my repeated patch sending :(


However, you have just said

> I already told you this didn't need to be done; please stop.


Actually, this is the first time you have told me to stop doing this.