Skip to contentSkip to conversation

inotify: Use strscpy() for event->name copies

PATCH In review Kees Cook 3 participants 4 messages Dec 16, 2024 – Dec 18, 2024

Subsystems (3)
VFS Fsnotify: filesystem notification infrastructure Inotify
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 inotify: Use strscpy() for event->name copies

Since we have already allocated "len + 1" space for event->name, make sure that name->name cannot ever accidentally cause a copy overflow by calling strscpy() instead of the unbounded strcpy() routine. This assists in the ongoing efforts to remove the unsafe strcpy() API[1] from the kernel.

Link https://github.com/KSPP/linux/issues/88 [1]
Signed-off-by Kees Cook <[email protected]>

---
Cc: Jan Kara <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: [email protected]

---
 fs/notify/inotify/inotify_fsnotify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/notify/inotify/inotify_fsnotify.c +1 -1
index 993375f0db67..cd7d11b0eb08 100644
@@ -121,7 +121,7 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 	event->sync_cookie = cookie;
 	event->name_len = len;
 	if (len)
-		strcpy(event->name, name->name);
+		strscpy(event->name, name->name, event->name_len + 1);
 
 	ret = fsnotify_add_event(group, fsn_event, inotify_merge);
 	if (ret) {
Signature · 1 line
2.34.1
#1 lore raw
Quoted 4 lines · Kees Cook

Since we have already allocated "len + 1" space for event->name, make sure that name->name cannot ever accidentally cause a copy overflow by calling strscpy() instead of the unbounded strcpy() routine. This assists in the ongoing efforts to remove the unsafe strcpy() API[1] from the kernel.

Since a qstr can't contain a NUL before the length, why not just use memcpy()?

Quoted 4 lines
fs/notify/inotify/inotify_fsnotify.c +1 -1
 	event->name_len = len;
 	if (len)
-		strcpy(event->name, name->name);
+		strscpy(event->name, name->name, event->name_len + 1);
author
#2 lore raw
Quoted 6 lines · Matthew Wilcox
Quoted 4 lines · Kees Cook

Since we have already allocated "len + 1" space for event->name, make sure that name->name cannot ever accidentally cause a copy overflow by calling strscpy() instead of the unbounded strcpy() routine. This assists in the ongoing efforts to remove the unsafe strcpy() API[1] from the kernel.

Quoted 7 lines

Since a qstr can't contain a NUL before the length, why not just use memcpy()?

fs/notify/inotify/inotify_fsnotify.c int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, · line 124

Quoted 4 lines
fs/notify/inotify/inotify_fsnotify.c +1 -1
 	event->name_len = len;
 	if (len)
-		strcpy(event->name, name->name);
+		strscpy(event->name, name->name, event->name_len + 1);

So that the destination is guaranteed to be NUL terminated no matter what's in the source. :) (i.e. try to limit unlikely conditions from expanding.)

Signature · 1 line
Kees Cook
maintainer
#3 lore raw
In reply to Kees Cook
Quoted 7 lines · Kees Cook

Since we have already allocated "len + 1" space for event->name, make sure that name->name cannot ever accidentally cause a copy overflow by calling strscpy() instead of the unbounded strcpy() routine. This assists in the ongoing efforts to remove the unsafe strcpy() API[1] from the kernel.

Link https://github.com/KSPP/linux/issues/88 [1]
Signed-off-by Kees Cook <[email protected]>

Thanks. Added to my tree.

Honza

Quoted 24 lines

---
Cc: Jan Kara <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: [email protected]

---
 fs/notify/inotify/inotify_fsnotify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/notify/inotify/inotify_fsnotify.c +1 -1
index 993375f0db67..cd7d11b0eb08 100644
@@ -121,7 +121,7 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 	event->sync_cookie = cookie;
 	event->name_len = len;
 	if (len)
-		strcpy(event->name, name->name);
+		strscpy(event->name, name->name, event->name_len + 1);
 
 	ret = fsnotify_add_event(group, fsn_event, inotify_merge);
 	if (ret) {
Signature · 1 line
2.34.1
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR