* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 3:54 [PATCH v2] ext4: replace strcpy() with '.' assignment Ethan Carter Edwards
@ 2025-05-19 5:58 ` kernel test robot
2025-05-19 13:56 ` Theodore Ts'o
2025-05-19 5:58 ` kernel test robot
2025-05-19 13:52 ` Kees Cook
2 siblings, 1 reply; 10+ messages in thread
From: kernel test robot @ 2025-05-19 5:58 UTC (permalink / raw)
To: Ethan Carter Edwards, Theodore Ts'o, Andreas Dilger
Cc: oe-kbuild-all, linux-ext4, linux-kernel, linux-hardening,
Ethan Carter Edwards
Hi Ethan,
kernel test robot noticed the following build errors:
[auto build test ERROR on 5723cc3450bccf7f98f227b9723b5c9f6b3af1c5]
url: https://github.com/intel-lab-lkp/linux/commits/Ethan-Carter-Edwards/ext4-replace-strcpy-with-assignment/20250519-115601
base: 5723cc3450bccf7f98f227b9723b5c9f6b3af1c5
patch link: https://lore.kernel.org/r/20250518-ext4-strcpy-v2-1-80d316325046%40ethancedwards.com
patch subject: [PATCH v2] ext4: replace strcpy() with '.' assignment
config: csky-randconfig-002-20250519 (https://download.01.org/0day-ci/archive/20250519/202505191316.JJMnPobO-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250519/202505191316.JJMnPobO-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505191316.JJMnPobO-lkp@intel.com/
All errors (new ones prefixed by >>):
fs/ext4/inline.c: In function 'ext4_inlinedir_to_tree':
>> fs/ext4/inline.c:1317:38: error: assignment to 'char' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
1317 | fake.name[0] = ".";
| ^
fs/ext4/inline.c:1327:53: error: assignment to 'char' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
1327 | fake.name[0] = fake.name[1] = ".";
| ^
vim +1317 fs/ext4/inline.c
1259
1260 /*
1261 * This function fills a red-black tree with information from an
1262 * inlined dir. It returns the number directory entries loaded
1263 * into the tree. If there is an error it is returned in err.
1264 */
1265 int ext4_inlinedir_to_tree(struct file *dir_file,
1266 struct inode *dir, ext4_lblk_t block,
1267 struct dx_hash_info *hinfo,
1268 __u32 start_hash, __u32 start_minor_hash,
1269 int *has_inline_data)
1270 {
1271 int err = 0, count = 0;
1272 unsigned int parent_ino;
1273 int pos;
1274 struct ext4_dir_entry_2 *de;
1275 struct inode *inode = file_inode(dir_file);
1276 int ret, inline_size = 0;
1277 struct ext4_iloc iloc;
1278 void *dir_buf = NULL;
1279 struct ext4_dir_entry_2 fake;
1280 struct fscrypt_str tmp_str;
1281
1282 ret = ext4_get_inode_loc(inode, &iloc);
1283 if (ret)
1284 return ret;
1285
1286 down_read(&EXT4_I(inode)->xattr_sem);
1287 if (!ext4_has_inline_data(inode)) {
1288 up_read(&EXT4_I(inode)->xattr_sem);
1289 *has_inline_data = 0;
1290 goto out;
1291 }
1292
1293 inline_size = ext4_get_inline_size(inode);
1294 dir_buf = kmalloc(inline_size, GFP_NOFS);
1295 if (!dir_buf) {
1296 ret = -ENOMEM;
1297 up_read(&EXT4_I(inode)->xattr_sem);
1298 goto out;
1299 }
1300
1301 ret = ext4_read_inline_data(inode, dir_buf, inline_size, &iloc);
1302 up_read(&EXT4_I(inode)->xattr_sem);
1303 if (ret < 0)
1304 goto out;
1305
1306 pos = 0;
1307 parent_ino = le32_to_cpu(((struct ext4_dir_entry_2 *)dir_buf)->inode);
1308 while (pos < inline_size) {
1309 /*
1310 * As inlined dir doesn't store any information about '.' and
1311 * only the inode number of '..' is stored, we have to handle
1312 * them differently.
1313 */
1314 if (pos == 0) {
1315 fake.inode = cpu_to_le32(inode->i_ino);
1316 fake.name_len = 1;
> 1317 fake.name[0] = ".";
1318 fake.rec_len = ext4_rec_len_to_disk(
1319 ext4_dir_rec_len(fake.name_len, NULL),
1320 inline_size);
1321 ext4_set_de_type(inode->i_sb, &fake, S_IFDIR);
1322 de = &fake;
1323 pos = EXT4_INLINE_DOTDOT_OFFSET;
1324 } else if (pos == EXT4_INLINE_DOTDOT_OFFSET) {
1325 fake.inode = cpu_to_le32(parent_ino);
1326 fake.name_len = 2;
1327 fake.name[0] = fake.name[1] = ".";
1328 fake.rec_len = ext4_rec_len_to_disk(
1329 ext4_dir_rec_len(fake.name_len, NULL),
1330 inline_size);
1331 ext4_set_de_type(inode->i_sb, &fake, S_IFDIR);
1332 de = &fake;
1333 pos = EXT4_INLINE_DOTDOT_SIZE;
1334 } else {
1335 de = (struct ext4_dir_entry_2 *)(dir_buf + pos);
1336 pos += ext4_rec_len_from_disk(de->rec_len, inline_size);
1337 if (ext4_check_dir_entry(inode, dir_file, de,
1338 iloc.bh, dir_buf,
1339 inline_size, pos)) {
1340 ret = count;
1341 goto out;
1342 }
1343 }
1344
1345 if (ext4_hash_in_dirent(dir)) {
1346 hinfo->hash = EXT4_DIRENT_HASH(de);
1347 hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
1348 } else {
1349 err = ext4fs_dirhash(dir, de->name, de->name_len, hinfo);
1350 if (err) {
1351 ret = err;
1352 goto out;
1353 }
1354 }
1355 if ((hinfo->hash < start_hash) ||
1356 ((hinfo->hash == start_hash) &&
1357 (hinfo->minor_hash < start_minor_hash)))
1358 continue;
1359 if (de->inode == 0)
1360 continue;
1361 tmp_str.name = de->name;
1362 tmp_str.len = de->name_len;
1363 err = ext4_htree_store_dirent(dir_file, hinfo->hash,
1364 hinfo->minor_hash, de, &tmp_str);
1365 if (err) {
1366 ret = err;
1367 goto out;
1368 }
1369 count++;
1370 }
1371 ret = count;
1372 out:
1373 kfree(dir_buf);
1374 brelse(iloc.bh);
1375 return ret;
1376 }
1377
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 5:58 ` kernel test robot
@ 2025-05-19 13:56 ` Theodore Ts'o
0 siblings, 0 replies; 10+ messages in thread
From: Theodore Ts'o @ 2025-05-19 13:56 UTC (permalink / raw)
To: Ethan Carter Edwards, Andreas Dilger, oe-kbuild-all, linux-ext4,
linux-kernel, linux-hardening
On Mon, May 19, 2025 at 01:58:02PM +0800, kernel test robot wrote:
> Hi Ethan,
>
> kernel test robot noticed the following build errors:
Hi Ethan,
I would really appreciate it if you would at least do a build test
before sending out a patch. In addition, it would also be helpful if
you ran a smoke test, using "kvm-xfstests smoke". The instructions
for how to use kvm-xfstests can be found here[1], and it was designed
to be as easy as possible for people who are sending "drive-by
patches".
[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
Running the smoketest only takes about 20 minutes; although if you
want to run more sophistcated testing there is also "kvm-xfstests -c
ext4/4k -g quick", or "kvm-xfstests -c ext4/4k -g auto", or if you
have a 24 hours to kill, there's always "kvm-xfststs full". (Although
these days I generally use gce-xfstests[2] since this can shard the test
runs across multiple VM's, so it only takes 2.5 hours of wall clock
time.)
[2] https://thunk.org/gce-xfstests
Cheers,
- TEd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 3:54 [PATCH v2] ext4: replace strcpy() with '.' assignment Ethan Carter Edwards
2025-05-19 5:58 ` kernel test robot
@ 2025-05-19 5:58 ` kernel test robot
2025-05-19 13:52 ` Kees Cook
2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-05-19 5:58 UTC (permalink / raw)
To: Ethan Carter Edwards, Theodore Ts'o, Andreas Dilger
Cc: oe-kbuild-all, linux-ext4, linux-kernel, linux-hardening,
Ethan Carter Edwards
Hi Ethan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5723cc3450bccf7f98f227b9723b5c9f6b3af1c5]
url: https://github.com/intel-lab-lkp/linux/commits/Ethan-Carter-Edwards/ext4-replace-strcpy-with-assignment/20250519-115601
base: 5723cc3450bccf7f98f227b9723b5c9f6b3af1c5
patch link: https://lore.kernel.org/r/20250518-ext4-strcpy-v2-1-80d316325046%40ethancedwards.com
patch subject: [PATCH v2] ext4: replace strcpy() with '.' assignment
config: csky-randconfig-001-20250519 (https://download.01.org/0day-ci/archive/20250519/202505191321.ZwdQZhut-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250519/202505191321.ZwdQZhut-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505191321.ZwdQZhut-lkp@intel.com/
All warnings (new ones prefixed by >>):
fs/ext4/inline.c: In function 'ext4_inlinedir_to_tree':
>> fs/ext4/inline.c:1317:17: warning: assignment to 'char' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
1317 | fake.name[0] = ".";
| ^
fs/ext4/inline.c:1327:32: warning: assignment to 'char' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
1327 | fake.name[0] = fake.name[1] = ".";
| ^
vim +1317 fs/ext4/inline.c
1259
1260 /*
1261 * This function fills a red-black tree with information from an
1262 * inlined dir. It returns the number directory entries loaded
1263 * into the tree. If there is an error it is returned in err.
1264 */
1265 int ext4_inlinedir_to_tree(struct file *dir_file,
1266 struct inode *dir, ext4_lblk_t block,
1267 struct dx_hash_info *hinfo,
1268 __u32 start_hash, __u32 start_minor_hash,
1269 int *has_inline_data)
1270 {
1271 int err = 0, count = 0;
1272 unsigned int parent_ino;
1273 int pos;
1274 struct ext4_dir_entry_2 *de;
1275 struct inode *inode = file_inode(dir_file);
1276 int ret, inline_size = 0;
1277 struct ext4_iloc iloc;
1278 void *dir_buf = NULL;
1279 struct ext4_dir_entry_2 fake;
1280 struct fscrypt_str tmp_str;
1281
1282 ret = ext4_get_inode_loc(inode, &iloc);
1283 if (ret)
1284 return ret;
1285
1286 down_read(&EXT4_I(inode)->xattr_sem);
1287 if (!ext4_has_inline_data(inode)) {
1288 up_read(&EXT4_I(inode)->xattr_sem);
1289 *has_inline_data = 0;
1290 goto out;
1291 }
1292
1293 inline_size = ext4_get_inline_size(inode);
1294 dir_buf = kmalloc(inline_size, GFP_NOFS);
1295 if (!dir_buf) {
1296 ret = -ENOMEM;
1297 up_read(&EXT4_I(inode)->xattr_sem);
1298 goto out;
1299 }
1300
1301 ret = ext4_read_inline_data(inode, dir_buf, inline_size, &iloc);
1302 up_read(&EXT4_I(inode)->xattr_sem);
1303 if (ret < 0)
1304 goto out;
1305
1306 pos = 0;
1307 parent_ino = le32_to_cpu(((struct ext4_dir_entry_2 *)dir_buf)->inode);
1308 while (pos < inline_size) {
1309 /*
1310 * As inlined dir doesn't store any information about '.' and
1311 * only the inode number of '..' is stored, we have to handle
1312 * them differently.
1313 */
1314 if (pos == 0) {
1315 fake.inode = cpu_to_le32(inode->i_ino);
1316 fake.name_len = 1;
> 1317 fake.name[0] = ".";
1318 fake.rec_len = ext4_rec_len_to_disk(
1319 ext4_dir_rec_len(fake.name_len, NULL),
1320 inline_size);
1321 ext4_set_de_type(inode->i_sb, &fake, S_IFDIR);
1322 de = &fake;
1323 pos = EXT4_INLINE_DOTDOT_OFFSET;
1324 } else if (pos == EXT4_INLINE_DOTDOT_OFFSET) {
1325 fake.inode = cpu_to_le32(parent_ino);
1326 fake.name_len = 2;
1327 fake.name[0] = fake.name[1] = ".";
1328 fake.rec_len = ext4_rec_len_to_disk(
1329 ext4_dir_rec_len(fake.name_len, NULL),
1330 inline_size);
1331 ext4_set_de_type(inode->i_sb, &fake, S_IFDIR);
1332 de = &fake;
1333 pos = EXT4_INLINE_DOTDOT_SIZE;
1334 } else {
1335 de = (struct ext4_dir_entry_2 *)(dir_buf + pos);
1336 pos += ext4_rec_len_from_disk(de->rec_len, inline_size);
1337 if (ext4_check_dir_entry(inode, dir_file, de,
1338 iloc.bh, dir_buf,
1339 inline_size, pos)) {
1340 ret = count;
1341 goto out;
1342 }
1343 }
1344
1345 if (ext4_hash_in_dirent(dir)) {
1346 hinfo->hash = EXT4_DIRENT_HASH(de);
1347 hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
1348 } else {
1349 err = ext4fs_dirhash(dir, de->name, de->name_len, hinfo);
1350 if (err) {
1351 ret = err;
1352 goto out;
1353 }
1354 }
1355 if ((hinfo->hash < start_hash) ||
1356 ((hinfo->hash == start_hash) &&
1357 (hinfo->minor_hash < start_minor_hash)))
1358 continue;
1359 if (de->inode == 0)
1360 continue;
1361 tmp_str.name = de->name;
1362 tmp_str.len = de->name_len;
1363 err = ext4_htree_store_dirent(dir_file, hinfo->hash,
1364 hinfo->minor_hash, de, &tmp_str);
1365 if (err) {
1366 ret = err;
1367 goto out;
1368 }
1369 count++;
1370 }
1371 ret = count;
1372 out:
1373 kfree(dir_buf);
1374 brelse(iloc.bh);
1375 return ret;
1376 }
1377
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 3:54 [PATCH v2] ext4: replace strcpy() with '.' assignment Ethan Carter Edwards
2025-05-19 5:58 ` kernel test robot
2025-05-19 5:58 ` kernel test robot
@ 2025-05-19 13:52 ` Kees Cook
2025-05-19 14:59 ` Theodore Ts'o
2 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2025-05-19 13:52 UTC (permalink / raw)
To: Ethan Carter Edwards
Cc: Theodore Ts'o, Andreas Dilger, linux-ext4, linux-kernel,
linux-hardening
On Sun, May 18, 2025 at 11:54:21PM -0400, Ethan Carter Edwards wrote:
> strcpy() is deprecated; assignment can be used instead which
> theoretically/potentially increases speed as a function call is removed.
>
> Straight assignment works because the strings are not null-terminated
> which means they don't strictly require a str(s)cpy call.
>
> No functional changes intended.
>
> Link: https://github.com/KSPP/linux/issues/88
> Suggested-by: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
> ---
> There's an ongoing effort to remove calls to strcpy throughout the
> kernel.
>
> Link: https://github.com/KSPP/linux/issues/88
> ---
> Changes in v2:
> - completely remove the call to strcpy and replace it with assignment
> off of Theo's suggestion. Thanks.
> - Link to v1: https://lore.kernel.org/r/20250518-ext4-strcpy-v1-1-6c8a82ff078f@ethancedwards.com
> ---
> fs/ext4/inline.c | 4 ++--
> fs/ext4/namei.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 2c9b762925c72f2ff5a402b02500370bc1eb0eb1..f3bc8b3904a8a9b55162f002b5bd63a527b290a5 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1314,7 +1314,7 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
> if (pos == 0) {
> fake.inode = cpu_to_le32(inode->i_ino);
> fake.name_len = 1;
> - strcpy(fake.name, ".");
> + fake.name[0] = ".";
This means the trailing NUL byte isn't being copied any more? That seems
like a big change, even if name_len is being used for length tracking.
--
Kees Cook
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 13:52 ` Kees Cook
@ 2025-05-19 14:59 ` Theodore Ts'o
2025-05-23 12:31 ` David Laight
0 siblings, 1 reply; 10+ messages in thread
From: Theodore Ts'o @ 2025-05-19 14:59 UTC (permalink / raw)
To: Kees Cook
Cc: Ethan Carter Edwards, Andreas Dilger, linux-ext4, linux-kernel,
linux-hardening
On Mon, May 19, 2025 at 06:52:13AM -0700, Kees Cook wrote:
> > --- a/fs/ext4/inline.c
> > +++ b/fs/ext4/inline.c
> > @@ -1314,7 +1314,7 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
> > if (pos == 0) {
> > fake.inode = cpu_to_le32(inode->i_ino);
> > fake.name_len = 1;
> > - strcpy(fake.name, ".");
> > + fake.name[0] = ".";
>
> This means the trailing NUL byte isn't being copied any more? That seems
> like a big change, even if name_len is being used for length tracking.
Yeah, and so that's something that needs to be tested (and not just
build tested to catch the obvious FTBFS bug). However, note how we
handle normal filenames, as opposed to "." and "..". From
ext4_insert_dentry():
de->inode = cpu_to_le32(inode->i_ino);
ext4_set_de_type(inode->i_sb, de, inode->i_mode);
de->name_len = fname_len(fname);
memcpy(de->name, fname_name(fname), fname_len(fname));
Or were you commenting on the "no functional changes intended" line in
the commit description? I agree that this is probably no longer
accurate. :-)
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-19 14:59 ` Theodore Ts'o
@ 2025-05-23 12:31 ` David Laight
2025-05-23 14:24 ` Theodore Ts'o
0 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2025-05-23 12:31 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Kees Cook, Ethan Carter Edwards, Andreas Dilger, linux-ext4,
linux-kernel, linux-hardening
On Mon, 19 May 2025 10:59:30 -0400
"Theodore Ts'o" <tytso@mit.edu> wrote:
> On Mon, May 19, 2025 at 06:52:13AM -0700, Kees Cook wrote:
> > > --- a/fs/ext4/inline.c
> > > +++ b/fs/ext4/inline.c
> > > @@ -1314,7 +1314,7 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
> > > if (pos == 0) {
> > > fake.inode = cpu_to_le32(inode->i_ino);
> > > fake.name_len = 1;
> > > - strcpy(fake.name, ".");
> > > + fake.name[0] = ".";
> >
> > This means the trailing NUL byte isn't being copied any more? That seems
> > like a big change, even if name_len is being used for length tracking.
>
> Yeah, and so that's something that needs to be tested (and not just
> build tested to catch the obvious FTBFS bug).
The compiler (or headers files) can also allow strcpy() of constant
length strings into arrays (known size). Erroring requests that are too long.
The strcpy() is then converted to a memcpy() which can then be optimised
into writes of constants.
So using strcpy() under those conditions 'isn't all bad' and can generate
better (and less bug prone) code than trying to hand-optimise it.
So even through strcpy() is usually a bad idea, there is not need to
remove the calls that the compiler can validate as safe.
David
> However, note how we
> handle normal filenames, as opposed to "." and "..". From
> ext4_insert_dentry():
>
> de->inode = cpu_to_le32(inode->i_ino);
> ext4_set_de_type(inode->i_sb, de, inode->i_mode);
> de->name_len = fname_len(fname);
> memcpy(de->name, fname_name(fname), fname_len(fname));
>
> Or were you commenting on the "no functional changes intended" line in
> the commit description? I agree that this is probably no longer
> accurate. :-)
>
> - Ted
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-23 12:31 ` David Laight
@ 2025-05-23 14:24 ` Theodore Ts'o
2025-05-23 17:14 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Theodore Ts'o @ 2025-05-23 14:24 UTC (permalink / raw)
To: David Laight
Cc: Kees Cook, Ethan Carter Edwards, Andreas Dilger, linux-ext4,
linux-kernel, linux-hardening
On Fri, May 23, 2025 at 01:31:00PM +0100, David Laight wrote:
>
> The compiler (or headers files) can also allow strcpy() of constant
> length strings into arrays (known size). Erroring requests that are too long.
> The strcpy() is then converted to a memcpy() which can then be optimised
> into writes of constants.
>
> So using strcpy() under those conditions 'isn't all bad' and can generate
> better (and less bug prone) code than trying to hand-optimise it.
>
> So even through strcpy() is usually a bad idea, there is not need to
> remove the calls that the compiler can validate as safe.
I assume that what the hardening folks want to do is to assert that
strcpy is always evil(tm) so they can detect potential security bugs
by doing "git grep strcpy".
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-23 14:24 ` Theodore Ts'o
@ 2025-05-23 17:14 ` Kees Cook
2025-05-23 22:07 ` Theodore Ts'o
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2025-05-23 17:14 UTC (permalink / raw)
To: Theodore Ts'o, David Laight
Cc: Ethan Carter Edwards, Andreas Dilger, linux-ext4, linux-kernel,
linux-hardening
On May 23, 2025 7:24:49 AM PDT, Theodore Ts'o <tytso@mit.edu> wrote:
>On Fri, May 23, 2025 at 01:31:00PM +0100, David Laight wrote:
>>
>> The compiler (or headers files) can also allow strcpy() of constant
>> length strings into arrays (known size). Erroring requests that are too long.
>> The strcpy() is then converted to a memcpy() which can then be optimised
>> into writes of constants.
>>
>> So using strcpy() under those conditions 'isn't all bad' and can generate
>> better (and less bug prone) code than trying to hand-optimise it.
>>
>> So even through strcpy() is usually a bad idea, there is not need to
>> remove the calls that the compiler can validate as safe.
>
>I assume that what the hardening folks want to do is to assert that
>strcpy is always evil(tm) so they can detect potential security bugs
>by doing "git grep strcpy".
FWIW, what I'd like is a lack of ambiguity for both humans and compilers. "Get rid of strcpy" is the Big Hammer solution for strcpy. The more precise version is "disallow strcpy of a src or dst where either lack a compile-time buffer size".
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ext4: replace strcpy() with '.' assignment
2025-05-23 17:14 ` Kees Cook
@ 2025-05-23 22:07 ` Theodore Ts'o
0 siblings, 0 replies; 10+ messages in thread
From: Theodore Ts'o @ 2025-05-23 22:07 UTC (permalink / raw)
To: Kees Cook
Cc: David Laight, Ethan Carter Edwards, Andreas Dilger, linux-ext4,
linux-kernel, linux-hardening
On Fri, May 23, 2025 at 10:14:04AM -0700, Kees Cook wrote:
>
>
> On May 23, 2025 7:24:49 AM PDT, Theodore Ts'o <tytso@mit.edu> wrote:
> >On Fri, May 23, 2025 at 01:31:00PM +0100, David Laight wrote:
> >>
> >> The compiler (or headers files) can also allow strcpy() of constant
> >> length strings into arrays (known size). Erroring requests that are too long.
> >> The strcpy() is then converted to a memcpy() which can then be optimised
> >> into writes of constants.
> >>
> >> So using strcpy() under those conditions 'isn't all bad' and can generate
> >> better (and less bug prone) code than trying to hand-optimise it.
> >>
> >> So even through strcpy() is usually a bad idea, there is not need to
> >> remove the calls that the compiler can validate as safe.
> >
> >I assume that what the hardening folks want to do is to assert that
> >strcpy is always evil(tm) so they can detect potential security bugs
> >by doing "git grep strcpy".
>
> FWIW, what I'd like is a lack of ambiguity for both humans and
> compilers. "Get rid of strcpy" is the Big Hammer solution for
> strcpy. The more precise version is "disallow strcpy of a src or dst
> where either lack a compile-time buffer size".
Well, technically speaking struct ext4_dir_entry.name has a fixed
compile-time buffer size:
struct ext4_dir_entry {
__le32 inode; /* Inode number */
__le16 rec_len; /* Directory entry length */
__le16 name_len; /* Name length */
char name[EXT4_NAME_LEN]; /* File name */
};
And what we're copying into name here is also fixed. It's either "."
or "..". As far as optimization is concerned, whether
de->name[0] = de->name[1] = '.';
could be better optimized by the compiler than:
strcpy(de->name, "..");
or
memcpy(de->name, "..", 2);
(which is all that is required)
Meh. Probably the compiler could optimized it into a 2-byte word
store, but it's not like mkdir is a hot path. :-)
It's probably easier to patch the code path and as opposed to having
the conversation about how "no, really, it's safe, and I can prove
it." If this was a performance hot path, I might care more, but it
isn't, so I don't.
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread