* [PATCH resend] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer
2024-11-10 6:41 [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer integral
@ 2024-12-09 18:15 ` Integral
2024-12-09 18:18 ` [PATCH resend v2] " Integral
2024-12-10 1:17 ` [PATCH] " Kent Overstreet
2 siblings, 0 replies; 5+ messages in thread
From: Integral @ 2024-12-09 18:15 UTC (permalink / raw)
To: integral
Cc: kent.overstreet, kent.overstreet, lihongbo22, linux-bcachefs,
mmpgouride
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH resend v2] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer
2024-11-10 6:41 [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer integral
2024-12-09 18:15 ` [PATCH resend] " Integral
@ 2024-12-09 18:18 ` Integral
2024-12-10 1:17 ` [PATCH] " Kent Overstreet
2 siblings, 0 replies; 5+ messages in thread
From: Integral @ 2024-12-09 18:18 UTC (permalink / raw)
To: integral
Cc: kent.overstreet, kent.overstreet, lihongbo22, linux-bcachefs,
mmpgouride
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer
2024-11-10 6:41 [PATCH] bcachefs: use strscpy & strscpy_pad to make string copying simpler & safer integral
2024-12-09 18:15 ` [PATCH resend] " Integral
2024-12-09 18:18 ` [PATCH resend v2] " Integral
@ 2024-12-10 1:17 ` Kent Overstreet
2024-12-10 5:07 ` Integral
2 siblings, 1 reply; 5+ messages in thread
From: Kent Overstreet @ 2024-12-10 1:17 UTC (permalink / raw)
To: integral@archlinuxcn.org
Cc: kent.overstreet, lihongbo22, mmpgouride, linux-bcachefs
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread