> On Jul 16, 2019, at 5:03 PM, Steven Rostedt <[email protected]> wrote: > > On Fri, 12 Jul 2019 12:14:47 -0400 > Qian Cai <[email protected]> wrote: > >> There are many of those warnings. >> >> In file included from ./arch/powerpc/include/asm/paca.h:15, >> from ./arch/powerpc/include/asm/current.h:13, >> from ./include/linux/thread_info.h:21, >> from ./include/asm-generic/preempt.h:5, >> from ./arch/powerpc/include/generated/asm/preempt.h:1, >> from ./include/linux/preempt.h:78, >> from ./include/linux/spinlock.h:51, >> from fs/fs-writeback.c:19: >> In function 'strncpy', >> inlined from 'perf_trace_writeback_page_template' at >> ./include/trace/events/writeback.h:56:1: >> ./include/linux/string.h:260:9: warning: '__builtin_strncpy' specified >> bound 32 equals destination size [-Wstringop-truncation] >> return __builtin_strncpy(p, q, size); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Fix it by using strlcpy() which will always be NUL-terminated instead of >> strncpy(). strlcpy() has already been used at some places in this file. >> >> Fixes: 455b2864686d ("writeback: Initial tracing support") >> Fixes: 028c2dd184c0 ("writeback: Add tracing to balance_dirty_pages") >> Fixes: e84d0a4f8e39 ("writeback: trace event writeback_queue_io") >> Fixes: b48c104d2211 ("writeback: trace event bdi_dirty_ratelimit") >> Fixes: cc1676d917f3 ("writeback: Move requeueing when I_SYNC set to >> writeback_sb_inodes()") >> Fixes: 9fb0a7da0c52 ("writeback: add more tracepoints") >> >> Signed-off-by: Qian Cai <[email protected]> >> --- >> include/trace/events/writeback.h | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/include/trace/events/writeback.h >> b/include/trace/events/writeback.h >> index aa7f3aeac740..8e3b3c4fd964 100644 >> --- a/include/trace/events/writeback.h >> +++ b/include/trace/events/writeback.h >> @@ -66,7 +66,7 @@ >> ), >> >> TP_fast_assign( >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : >> "(unknown)", 32); > > > Not sure this is an issue or not, but although the fix looks legit (in > case a string is more that 31 bytes), strlcpy() does not pad the rest > of the string like strncpy() does. This means we can possibly leak data > through the ring buffer. > > This may not be an issue as ftrace can only be used by a super user > account, but this code can also be used by perf. If it is possible for > a non admin account to enable these events through perf, then there is > a case of data leak. > > Again, it may not be a big issue, but I'm just letting people know. > > Note, this needs to go through the maintainer of the writeback.h, who > are those that created it, not the tracing maintainers.
Adding Jens. He handled those patches in the past. The original patch is here, https://lkml.kernel.org/lkml/[email protected]/ > > -- Steve > > >> __entry->ino = mapping ? mapping->host->i_ino : 0; >> __entry->index = page->index; >> @@ -110,7 +110,7 @@ >> struct backing_dev_info *bdi = inode_to_bdi(inode); >> >> /* may be called for files on pseudo FSes w/ unregistered bdi */ >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32); >> __entry->ino = inode->i_ino; >> __entry->state = inode->i_state; >> @@ -190,7 +190,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> ), >> >> TP_fast_assign( >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> dev_name(inode_to_bdi(inode)->dev), 32); >> __entry->ino = inode->i_ino; >> __entry->sync_mode = wbc->sync_mode; >> @@ -234,7 +234,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> __field(unsigned int, cgroup_ino) >> ), >> TP_fast_assign( >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", >> 32); >> __entry->nr_pages = work->nr_pages; >> __entry->sb_dev = work->sb ? work->sb->s_dev : 0; >> @@ -288,7 +288,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> __field(unsigned int, cgroup_ino) >> ), >> TP_fast_assign( >> - strncpy(__entry->name, dev_name(wb->bdi->dev), 32); >> + strlcpy(__entry->name, dev_name(wb->bdi->dev), 32); >> __entry->cgroup_ino = __trace_wb_assign_cgroup(wb); >> ), >> TP_printk("bdi %s: cgroup_ino=%u", >> @@ -310,7 +310,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> __array(char, name, 32) >> ), >> TP_fast_assign( >> - strncpy(__entry->name, dev_name(bdi->dev), 32); >> + strlcpy(__entry->name, dev_name(bdi->dev), 32); >> ), >> TP_printk("bdi %s", >> __entry->name >> @@ -335,7 +335,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> ), >> >> TP_fast_assign( >> - strncpy(__entry->name, dev_name(bdi->dev), 32); >> + strlcpy(__entry->name, dev_name(bdi->dev), 32); >> __entry->nr_to_write = wbc->nr_to_write; >> __entry->pages_skipped = wbc->pages_skipped; >> __entry->sync_mode = wbc->sync_mode; >> @@ -386,7 +386,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> ), >> TP_fast_assign( >> unsigned long *older_than_this = work->older_than_this; >> - strncpy(__entry->name, dev_name(wb->bdi->dev), 32); >> + strlcpy(__entry->name, dev_name(wb->bdi->dev), 32); >> __entry->older = older_than_this ? *older_than_this : 0; >> __entry->age = older_than_this ? >> (jiffies - *older_than_this) * 1000 / HZ : -1; >> @@ -597,7 +597,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> ), >> >> TP_fast_assign( >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> dev_name(inode_to_bdi(inode)->dev), 32); >> __entry->ino = inode->i_ino; >> __entry->state = inode->i_state; >> @@ -671,7 +671,7 @@ static inline unsigned int >> __trace_wbc_assign_cgroup(struct writeback_control *w >> ), >> >> TP_fast_assign( >> - strncpy(__entry->name, >> + strlcpy(__entry->name, >> dev_name(inode_to_bdi(inode)->dev), 32); >> __entry->ino = inode->i_ino; >> __entry->state = inode->i_state;
