| Message ID | 20241216224507.work.859-kees@kernel.org (mailing list archive) |
|---|---|
| State | In Next |
| Commit | b8f2688258f886f0bc0c0cb3ebe51efaa12191ec |
| Headers | show |
| Series | inotify: Use strscpy() for event->name copies | expand |
On Mon, Dec 16, 2024 at 02:45:15PM -0800, Kees Cook wrote: > 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()? > event->name_len = len; > if (len) > - strcpy(event->name, name->name); > + strscpy(event->name, name->name, event->name_len + 1);
On December 16, 2024 8:12:07 PM PST, Matthew Wilcox <willy@infradead.org> wrote: >On Mon, Dec 16, 2024 at 02:45:15PM -0800, Kees Cook wrote: >> 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()? > >> 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.)
On Mon 16-12-24 14:45:15, Kees Cook wrote: > 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 <kees@kernel.org> Thanks. Added to my tree. Honza > --- > Cc: Jan Kara <jack@suse.cz> > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: linux-fsdevel@vger.kernel.org > --- > fs/notify/inotify/inotify_fsnotify.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > index 993375f0db67..cd7d11b0eb08 100644 > --- a/fs/notify/inotify/inotify_fsnotify.c > +++ b/fs/notify/inotify/inotify_fsnotify.c > @@ -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) { > -- > 2.34.1 >
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c index 993375f0db67..cd7d11b0eb08 100644 --- a/fs/notify/inotify/inotify_fsnotify.c +++ b/fs/notify/inotify/inotify_fsnotify.c @@ -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) {
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 <kees@kernel.org> --- Cc: Jan Kara <jack@suse.cz> Cc: Amir Goldstein <amir73il@gmail.com> Cc: linux-fsdevel@vger.kernel.org --- fs/notify/inotify/inotify_fsnotify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)