[PATCH] fs/xfs: replace strncpy with strscpy

[PATCH] fs/xfs: replace strncpy with strscpy

[Pranav Tyagi]

Replace the deprecated strncpy() with strscpy() as the destination
buffer should be NUL-terminated and does not require any trailing
NUL-padding. Also, since NUL-termination is guaranteed,
use sizeof(label) in place of XFSLABEL_MAX as the size
parameter.

Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
---
 fs/xfs/xfs_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d250f7f74e3b..9f4d68c5b5ab 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -992,7 +992,7 @@ xfs_ioc_getlabel(
 	/* 1 larger than sb_fname, so this ensures a trailing NUL char */
 	memset(label, 0, sizeof(label));
 	spin_lock(&mp->m_sb_lock);
-	strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
+	strscpy(label, sbp->sb_fname, sizeof(label));
 	spin_unlock(&mp->m_sb_lock);
 
 	if (copy_to_user(user_label, label, sizeof(label)))
-- 
2.49.0

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[kernel test robot]

Hello,

kernel test robot noticed "WARNING:at_lib/string_helpers.c:#__fortify_report" on:

commit: 977e0a4036f9aa8cbbe33973e1eb7a1924191a5a ("[PATCH] fs/xfs: replace strncpy with strscpy")
url: https://github.com/intel-lab-lkp/linux/commits/Pranav-Tyagi/fs-xfs-replace-strncpy-with-strscpy/20250617-204752
base: https://git.kernel.org/cgit/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/all/20250617124546.24102-1-pranav.tyagi03@gmail.com/
patch subject: [PATCH] fs/xfs: replace strncpy with strscpy

in testcase: xfstests
version: xfstests-x86_64-b3da4865-1_20250623
with following parameters:

	disk: 4HDD
	fs: xfs
	test: xfs-group-50



config: x86_64-rhel-9.4-func
compiler: gcc-12
test machine: 4 threads Intel(R) Xeon(R) CPU E3-1225 v5 @ 3.30GHz (Skylake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202506300953.8b18c4e0-lkp@intel.com


[  167.002786][ T7295] ------------[ cut here ]------------
[  167.008126][ T7295] strnlen: detected buffer overflow: 13 byte read of buffer size 12
[ 167.016029][ T7295] WARNING: CPU: 3 PID: 7295 at lib/string_helpers.c:1035 __fortify_report (lib/string_helpers.c:1035) 
[  167.025263][ T7295] Modules linked in: xfs btrfs intel_rapl_msr blake2b_generic intel_rapl_common xor zstd_compress snd_hda_codec_hdmi raid6_pq x86_pkg_temp_thermal snd_soc_avs intel_powerclamp snd_soc_hda_codec snd_hda_codec_realtek coretemp snd_hda_codec_generic snd_hda_ext_core snd_hda_scodec_component sd_mod kvm_intel sg snd_soc_core ipmi_devintf ipmi_msghandler i915 snd_compress kvm snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec intel_gtt cec snd_hda_core irqbypass ghash_clmulni_intel sha512_ssse3 snd_hwdep drm_buddy sha1_ssse3 snd_pcm mei_wdt ttm rapl ahci intel_cstate wmi_bmof snd_timer drm_display_helper libahci mei_me drm_client_lib snd mei intel_uncore drm_kms_helper ie31200_edac libata pcspkr soundcore serio_raw i2c_i801 video i2c_smbus intel_pch_thermal intel_pmc_core pmt_telemetry wmi pmt_class acpi_pad intel_pmc_ssram_telemetry intel_vsec binfmt_misc loop fuse drm dm_mod ip_tables
[  167.106191][ T7295] CPU: 3 UID: 0 PID: 7295 Comm: xfs_io Tainted: G S                  6.16.0-rc2-00007-g977e0a4036f9 #1 PREEMPT(voluntary)
[  167.118796][ T7295] Tainted: [S]=CPU_OUT_OF_SPEC
[  167.123437][ T7295] Hardware name: HP HP Z238 Microtower Workstation/8183, BIOS N51 Ver. 01.63 10/05/2017
[ 167.133014][ T7295] RIP: 0010:__fortify_report (lib/string_helpers.c:1035) 
[ 167.138351][ T7295] Code: 59 40 84 ed 48 c7 c0 00 77 54 84 48 c7 c1 40 77 54 84 48 8b 34 dd 00 84 54 84 48 0f 44 c8 48 c7 c7 80 77 54 84 e8 c1 57 c3 fe <0f> 0b 48 83 c4 10 5b 5d c3 cc cc cc cc 48 89 34 24 48 c7 c7 a0 75
All code
========
   0:	59                   	pop    %rcx
   1:	40 84 ed             	test   %bpl,%bpl
   4:	48 c7 c0 00 77 54 84 	mov    $0xffffffff84547700,%rax
   b:	48 c7 c1 40 77 54 84 	mov    $0xffffffff84547740,%rcx
  12:	48 8b 34 dd 00 84 54 	mov    -0x7bab7c00(,%rbx,8),%rsi
  19:	84 
  1a:	48 0f 44 c8          	cmove  %rax,%rcx
  1e:	48 c7 c7 80 77 54 84 	mov    $0xffffffff84547780,%rdi
  25:	e8 c1 57 c3 fe       	call   0xfffffffffec357eb
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	48 83 c4 10          	add    $0x10,%rsp
  30:	5b                   	pop    %rbx
  31:	5d                   	pop    %rbp
  32:	c3                   	ret
  33:	cc                   	int3
  34:	cc                   	int3
  35:	cc                   	int3
  36:	cc                   	int3
  37:	48 89 34 24          	mov    %rsi,(%rsp)
  3b:	48                   	rex.W
  3c:	c7                   	.byte 0xc7
  3d:	c7                   	(bad)
  3e:	a0                   	.byte 0xa0
  3f:	75                   	.byte 0x75

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	48 83 c4 10          	add    $0x10,%rsp
   6:	5b                   	pop    %rbx
   7:	5d                   	pop    %rbp
   8:	c3                   	ret
   9:	cc                   	int3
   a:	cc                   	int3
   b:	cc                   	int3
   c:	cc                   	int3
   d:	48 89 34 24          	mov    %rsi,(%rsp)
  11:	48                   	rex.W
  12:	c7                   	.byte 0xc7
  13:	c7                   	(bad)
  14:	a0                   	.byte 0xa0
  15:	75                   	.byte 0x75
[  167.157828][ T7295] RSP: 0018:ffffc90009d6fa10 EFLAGS: 00010286
[  167.163765][ T7295] RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffffffff81931ee5
[  167.171607][ T7295] RDX: 1ffff1106f236180 RSI: 0000000000000008 RDI: ffff8883791b0c00
[  167.179457][ T7295] RBP: 0000000000000000 R08: 0000000000000001 R09: fffff520013adefd
[  167.187300][ T7295] R10: ffffc90009d6f7ef R11: 0000000000000001 R12: ffff8881731be540
[  167.195139][ T7295] R13: 00007ffd1f891350 R14: ffff8881731be06c R15: ffff8881c6ef8138
[  167.202998][ T7295] FS:  00007f6019ba0840(0000) GS:ffff8883f1da6000(0000) knlGS:0000000000000000
[  167.211808][ T7295] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  167.218267][ T7295] CR2: 00007f6019b9fd58 CR3: 0000000248c0c004 CR4: 00000000003726f0
[  167.226104][ T7295] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  167.233955][ T7295] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  167.241801][ T7295] Call Trace:
[  167.244960][ T7295]  <TASK>
[ 167.247767][ T7295] ? __asan_memset (mm/kasan/shadow.c:84) 
[ 167.252239][ T7295] __fortify_panic (lib/string_helpers.c:1043) 
[ 167.256593][ T7295] xfs_file_ioctl (fs/xfs/xfs_ioctl.c:1420) xfs 
[ 167.262288][ T7295] ? __pfx_xfs_file_ioctl (fs/xfs/xfs_ioctl.c:1188) xfs 
[ 167.268213][ T7295] ? put_pid (kernel/pid.c:332 kernel/pid.c:459) 
[ 167.272850][ T7295] ? kernel_clone (kernel/fork.c:2559) 
[ 167.277398][ T7295] ? __pfx_kernel_clone (kernel/fork.c:2559) 
[ 167.282288][ T7295] ? recalc_sigpending (arch/x86/include/asm/bitops.h:206 arch/x86/include/asm/bitops.h:238 include/asm-generic/bitops/instrumented-non-atomic.h:142 include/linux/thread_info.h:126 kernel/signal.c:180) 
[ 167.287262][ T7295] ? __do_sys_clone3 (kernel/fork.c:2903) 
[ 167.292071][ T7295] ? __pfx_do_vfs_ioctl (fs/ioctl.c:804) 
[ 167.296962][ T7295] ? _raw_spin_lock_irq (arch/x86/include/asm/atomic.h:107 include/linux/atomic/atomic-arch-fallback.h:2170 include/linux/atomic/atomic-instrumented.h:1302 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:187 include/linux/spinlock_api_smp.h:120 kernel/locking/spinlock.c:170) 
[ 167.301866][ T7295] ? __pfx__raw_spin_lock_irq (kernel/locking/spinlock.c:169) 
[ 167.307276][ T7295] ? recalc_sigpending (arch/x86/include/asm/bitops.h:206 arch/x86/include/asm/bitops.h:238 include/asm-generic/bitops/instrumented-non-atomic.h:142 include/linux/thread_info.h:126 kernel/signal.c:180) 
[ 167.312252][ T7295] ? sigprocmask (kernel/signal.c:3259) 
[ 167.316712][ T7295] ? __pfx_sigprocmask (kernel/signal.c:3236) 
[ 167.321514][ T7295] ? fdget (include/linux/file.h:57 fs/file.c:1161 fs/file.c:1166) 
[ 167.325444][ T7295] __x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:907 fs/ioctl.c:893 fs/ioctl.c:893) 
[ 167.330078][ T7295] do_syscall_64 (arch/x86/entry/syscall_64.c:63 arch/x86/entry/syscall_64.c:94) 
[ 167.334451][ T7295] ? do_syscall_64 (arch/x86/entry/syscall_64.c:63 arch/x86/entry/syscall_64.c:94) 
[ 167.339007][ T7295] ? handle_mm_fault (mm/memory.c:6274 mm/memory.c:6427) 
[ 167.343810][ T7295] ? do_user_addr_fault (arch/x86/include/asm/atomic.h:93 include/linux/atomic/atomic-arch-fallback.h:949 include/linux/atomic/atomic-instrumented.h:401 include/linux/refcount.h:389 include/linux/refcount.h:432 include/linux/mmap_lock.h:142 include/linux/mmap_lock.h:237 arch/x86/mm/fault.c:1338) 
[ 167.348872][ T7295] ? exc_page_fault (arch/x86/include/asm/irqflags.h:37 arch/x86/include/asm/irqflags.h:114 arch/x86/mm/fault.c:1484 arch/x86/mm/fault.c:1532) 
[ 167.353412][ T7295] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
[  167.359161][ T7295] RIP: 0033:0x7f6019ed0d1b
[ 167.363441][ T7295] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
All code
========
   0:	00 48 89             	add    %cl,-0x77(%rax)
   3:	44 24 18             	rex.R and $0x18,%al
   6:	31 c0                	xor    %eax,%eax
   8:	48 8d 44 24 60       	lea    0x60(%rsp),%rax
   d:	c7 04 24 10 00 00 00 	movl   $0x10,(%rsp)
  14:	48 89 44 24 08       	mov    %rax,0x8(%rsp)
  19:	48 8d 44 24 20       	lea    0x20(%rsp),%rax
  1e:	48 89 44 24 10       	mov    %rax,0x10(%rsp)
  23:	b8 10 00 00 00       	mov    $0x10,%eax
  28:	0f 05                	syscall
  2a:*	89 c2                	mov    %eax,%edx		<-- trapping instruction
  2c:	3d 00 f0 ff ff       	cmp    $0xfffff000,%eax
  31:	77 1c                	ja     0x4f
  33:	48 8b 44 24 18       	mov    0x18(%rsp),%rax
  38:	64                   	fs
  39:	48                   	rex.W
  3a:	2b                   	.byte 0x2b
  3b:	04 25                	add    $0x25,%al
  3d:	28 00                	sub    %al,(%rax)
	...

Code starting with the faulting instruction
===========================================
   0:	89 c2                	mov    %eax,%edx
   2:	3d 00 f0 ff ff       	cmp    $0xfffff000,%eax
   7:	77 1c                	ja     0x25
   9:	48 8b 44 24 18       	mov    0x18(%rsp),%rax
   e:	64                   	fs
   f:	48                   	rex.W
  10:	2b                   	.byte 0x2b
  11:	04 25                	add    $0x25,%al
  13:	28 00                	sub    %al,(%rax)


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250630/202506300953.8b18c4e0-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Carlos Maiolino]

On Tue, Jun 17, 2025 at 06:15:46PM +0530, Pranav Tyagi wrote:
> Replace the deprecated strncpy() with strscpy() as the destination
> buffer should be NUL-terminated and does not require any trailing
> NUL-padding. Also, since NUL-termination is guaranteed,

NUL-termination is only guaranteed if you copy into the buffer one less
byte than the label requires, i.e XFSLABEL_MAX.

> use sizeof(label) in place of XFSLABEL_MAX as the size
> parameter.

This is wrong, see below why.

> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> ---
>  fs/xfs/xfs_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d250f7f74e3b..9f4d68c5b5ab 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -992,7 +992,7 @@ xfs_ioc_getlabel(
>  	/* 1 larger than sb_fname, so this ensures a trailing NUL char */
>  	memset(label, 0, sizeof(label));
>  	spin_lock(&mp->m_sb_lock);
> -	strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> +	strscpy(label, sbp->sb_fname, sizeof(label));

This is broken and you created a buffer overrun here.

XFSLABEL_MAX is set to 12 bytes. The current label size is 13 bytes:

char                    label[XFSLABEL_MAX + 1];

This ensures the label will always have a null termination character as
long as you copy XFSLABEL_MAX bytes into the label.

- strncpy(label, sbp->sb_fname, XFSLABEL_MAX);

Copies 12 bytes from sb_fname into label. This ensures we always have a
trailing \0 at the last byte.

Your version:

strscpy(label, sbp->sb_fname, sizeof(label));

Copies 13 bytes from sb_fname into the label buffer.

This not only could have copied a non-null byte to the last byte in the
label buffer, but also But sbp->sb_fname size is XFSLABEL_MAX, so you
are reading beyond the source buffer size, causing a buffer overrun as you
can see on the kernel test robot report.

Carlos

>  	spin_unlock(&mp->m_sb_lock);
> 
>  	if (copy_to_user(user_label, label, sizeof(label)))
> --
> 2.49.0
> 

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Pranav Tyagi]

On Mon, Jun 30, 2025 at 2:09 PM Carlos Maiolino <cem@kernel.org> wrote:
>
> On Tue, Jun 17, 2025 at 06:15:46PM +0530, Pranav Tyagi wrote:
> > Replace the deprecated strncpy() with strscpy() as the destination
> > buffer should be NUL-terminated and does not require any trailing
> > NUL-padding. Also, since NUL-termination is guaranteed,
>
> NUL-termination is only guaranteed if you copy into the buffer one less
> byte than the label requires, i.e XFSLABEL_MAX.
>
> > use sizeof(label) in place of XFSLABEL_MAX as the size
> > parameter.
>
> This is wrong, see below why.
>
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index d250f7f74e3b..9f4d68c5b5ab 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -992,7 +992,7 @@ xfs_ioc_getlabel(
> >       /* 1 larger than sb_fname, so this ensures a trailing NUL char */
> >       memset(label, 0, sizeof(label));
> >       spin_lock(&mp->m_sb_lock);
> > -     strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> > +     strscpy(label, sbp->sb_fname, sizeof(label));
>
> This is broken and you created a buffer overrun here.
>
> XFSLABEL_MAX is set to 12 bytes. The current label size is 13 bytes:
>
> char                    label[XFSLABEL_MAX + 1];
>
> This ensures the label will always have a null termination character as
> long as you copy XFSLABEL_MAX bytes into the label.
>
> - strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
>
> Copies 12 bytes from sb_fname into label. This ensures we always have a
> trailing \0 at the last byte.
>
> Your version:
>
> strscpy(label, sbp->sb_fname, sizeof(label));
>
> Copies 13 bytes from sb_fname into the label buffer.
>
> This not only could have copied a non-null byte to the last byte in the
> label buffer, but also But sbp->sb_fname size is XFSLABEL_MAX, so you
> are reading beyond the source buffer size, causing a buffer overrun as you
> can see on the kernel test robot report.
>
> Carlos
>
> >       spin_unlock(&mp->m_sb_lock);
> >
> >       if (copy_to_user(user_label, label, sizeof(label)))
> > --
> > 2.49.0
> >

Hi,

Thank you for the feedback. I understand that my patch is incorrect and
it causes a buffer overrun. The destination buffer is indeed, already, null
terminated. Would you like me to send a corrected patch which uses
strscpy() (as strncpy() is deprecated)?

Regret the inconvenience.

Regards
Pranav Tyagi

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Brahmajit Das]

On 30.06.2025 14:36, Pranav Tyagi wrote:
... snip ...
> > >       spin_unlock(&mp->m_sb_lock);
> > >
> > >       if (copy_to_user(user_label, label, sizeof(label)))
> > > --
> > > 2.49.0
> > >
> 
> Hi,
> 
> Thank you for the feedback. I understand that my patch is incorrect and
> it causes a buffer overrun. The destination buffer is indeed, already, null
> terminated. Would you like me to send a corrected patch which uses
> strscpy() (as strncpy() is deprecated)?
> 
If the destination buffer is already NUL terminated, you can use either
strtomem or strtomem_pad [0].

[0]: https://docs.kernel.org/core-api/kernel-api.html#c.strncpy
(Description)
> Regret the inconvenience.
> 
> Regards
> Pranav Tyagi
> 

-- 
Regards,
listout

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Pranav Tyagi]

On Mon, Jun 30, 2025 at 7:48 PM Brahmajit Das <listout@listout.xyz> wrote:
>
> On 30.06.2025 14:36, Pranav Tyagi wrote:
> ... snip ...
> > > >       spin_unlock(&mp->m_sb_lock);
> > > >
> > > >       if (copy_to_user(user_label, label, sizeof(label)))
> > > > --
> > > > 2.49.0
> > > >
> >
> > Hi,
> >
> > Thank you for the feedback. I understand that my patch is incorrect and
> > it causes a buffer overrun. The destination buffer is indeed, already, null
> > terminated. Would you like me to send a corrected patch which uses
> > strscpy() (as strncpy() is deprecated)?
> >
> If the destination buffer is already NUL terminated, you can use either
> strtomem or strtomem_pad [0].
>
> [0]: https://docs.kernel.org/core-api/kernel-api.html#c.strncpy
> (Description)

Thank you for the suggestion. On going through [0], I see that,
both, strtomem and strscpy require the src to be null
terminated. As far as I know, sb_fname buffer has a size of
XFSLABEL_MAX (12 bytes). This means no terminating NULL
for the src. So shouldn't memcpy() be the right function to use here?

> > Regret the inconvenience.
> >
> > Regards
> > Pranav Tyagi
> >
>
> --
> Regards,
> listout

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 02:18:12PM +0530, Pranav Tyagi wrote:
> On Mon, Jun 30, 2025 at 7:48 PM Brahmajit Das <listout@listout.xyz> wrote:
> >
> > On 30.06.2025 14:36, Pranav Tyagi wrote:
> > ... snip ...
> > > > >       spin_unlock(&mp->m_sb_lock);
> > > > >
> > > > >       if (copy_to_user(user_label, label, sizeof(label)))
> > > > > --
> > > > > 2.49.0
> > > > >
> > >
> > > Hi,
> > >
> > > Thank you for the feedback. I understand that my patch is incorrect and
> > > it causes a buffer overrun. The destination buffer is indeed, already, null
> > > terminated. Would you like me to send a corrected patch which uses
> > > strscpy() (as strncpy() is deprecated)?
> > >
> > If the destination buffer is already NUL terminated, you can use either
> > strtomem or strtomem_pad [0].
> >
> > [0]: https://docs.kernel.org/core-api/kernel-api.html#c.strncpy
> > (Description)
> 
> Thank you for the suggestion. On going through [0], I see that,
> both, strtomem and strscpy require the src to be null
> terminated. As far as I know, sb_fname buffer has a size of
> XFSLABEL_MAX (12 bytes). This means no terminating NULL
> for the src. So shouldn't memcpy() be the right function to use here?

memtostr_pad?

--D

> > > Regret the inconvenience.
> > >
> > > Regards
> > > Pranav Tyagi
> > >
> >
> > --
> > Regards,
> > listout
> 

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Pranav Tyagi]

On Tue, Jul 1, 2025 at 8:27 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 02:18:12PM +0530, Pranav Tyagi wrote:
> > On Mon, Jun 30, 2025 at 7:48 PM Brahmajit Das <listout@listout.xyz> wrote:
> > >
> > > On 30.06.2025 14:36, Pranav Tyagi wrote:
> > > ... snip ...
> > > > > >       spin_unlock(&mp->m_sb_lock);
> > > > > >
> > > > > >       if (copy_to_user(user_label, label, sizeof(label)))
> > > > > > --
> > > > > > 2.49.0
> > > > > >
> > > >
> > > > Hi,
> > > >
> > > > Thank you for the feedback. I understand that my patch is incorrect and
> > > > it causes a buffer overrun. The destination buffer is indeed, already, null
> > > > terminated. Would you like me to send a corrected patch which uses
> > > > strscpy() (as strncpy() is deprecated)?
> > > >
> > > If the destination buffer is already NUL terminated, you can use either
> > > strtomem or strtomem_pad [0].
> > >
> > > [0]: https://docs.kernel.org/core-api/kernel-api.html#c.strncpy
> > > (Description)
> >
> > Thank you for the suggestion. On going through [0], I see that,
> > both, strtomem and strscpy require the src to be null
> > terminated. As far as I know, sb_fname buffer has a size of
> > XFSLABEL_MAX (12 bytes). This means no terminating NULL
> > for the src. So shouldn't memcpy() be the right function to use here?
>
> memtostr_pad?
>
> --D
>

Thanks for the suggestion. memtostr_pad fits perfectly for this use case.
It takes care of null termination as well as avoids the need for separate
zeroing. I'll update the patch accordingly and resend it.

Regards
Pranav Tyagi

> > > > Regret the inconvenience.
> > > >
> > > > Regards
> > > > Pranav Tyagi
> > > >
> > >
> > > --
> > > Regards,
> > > listout
> >

Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Carlos Maiolino]

On Mon, Jun 30, 2025 at 02:36:01PM +0530, Pranav Tyagi wrote:
> On Mon, Jun 30, 2025 at 2:09 PM Carlos Maiolino <cem@kernel.org> wrote:
> >
> > On Tue, Jun 17, 2025 at 06:15:46PM +0530, Pranav Tyagi wrote:
> > > Replace the deprecated strncpy() with strscpy() as the destination
> > > buffer should be NUL-terminated and does not require any trailing
> > > NUL-padding. Also, since NUL-termination is guaranteed,
> >
> > NUL-termination is only guaranteed if you copy into the buffer one less
> > byte than the label requires, i.e XFSLABEL_MAX.
> >
> > > use sizeof(label) in place of XFSLABEL_MAX as the size
> > > parameter.
> >
> > This is wrong, see below why.
> >
> > >
> > > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > > ---
> > >  fs/xfs/xfs_ioctl.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index d250f7f74e3b..9f4d68c5b5ab 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -992,7 +992,7 @@ xfs_ioc_getlabel(
> > >       /* 1 larger than sb_fname, so this ensures a trailing NUL char */
> > >       memset(label, 0, sizeof(label));
> > >       spin_lock(&mp->m_sb_lock);
> > > -     strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> > > +     strscpy(label, sbp->sb_fname, sizeof(label));
> >
> > This is broken and you created a buffer overrun here.
> >
> > XFSLABEL_MAX is set to 12 bytes. The current label size is 13 bytes:
> >
> > char                    label[XFSLABEL_MAX + 1];
> >
> > This ensures the label will always have a null termination character as
> > long as you copy XFSLABEL_MAX bytes into the label.
> >
> > - strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> >
> > Copies 12 bytes from sb_fname into label. This ensures we always have a
> > trailing \0 at the last byte.
> >
> > Your version:
> >
> > strscpy(label, sbp->sb_fname, sizeof(label));
> >
> > Copies 13 bytes from sb_fname into the label buffer.
> >
> > This not only could have copied a non-null byte to the last byte in the
> > label buffer, but also But sbp->sb_fname size is XFSLABEL_MAX, so you
> > are reading beyond the source buffer size, causing a buffer overrun as you
> > can see on the kernel test robot report.
> >
> > Carlos
> >
> > >       spin_unlock(&mp->m_sb_lock);
> > >
> > >       if (copy_to_user(user_label, label, sizeof(label)))
> > > --
> > > 2.49.0
> > >
> 
> Hi,
> 
> Thank you for the feedback. I understand that my patch is incorrect and
> it causes a buffer overrun. The destination buffer is indeed, already, null
> terminated. Would you like me to send a corrected patch which uses
> strscpy() (as strncpy() is deprecated)?

Sure, do so.

Carlos

> 
> Regret the inconvenience.
> 
> Regards
> Pranav Tyagi