* [PATCH] trace/writeback: fix Wstringop-truncation warnings
@ 2019-07-12 16:14 Qian Cai
2019-07-16 21:03 ` Steven Rostedt
0 siblings, 1 reply; 4+ messages in thread
From: Qian Cai @ 2019-07-12 16:14 UTC (permalink / raw)
To: rostedt, mingo; +Cc: tj, dchinner, fengguang.wu, jack, linux-kernel, Qian Cai
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 <cai@lca.pw>
---
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);
__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;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings
2019-07-12 16:14 [PATCH] trace/writeback: fix Wstringop-truncation warnings Qian Cai
@ 2019-07-16 21:03 ` Steven Rostedt
2019-07-18 20:11 ` Qian Cai
2019-07-29 10:04 ` Jan Kara
0 siblings, 2 replies; 4+ messages in thread
From: Steven Rostedt @ 2019-07-16 21:03 UTC (permalink / raw)
To: Qian Cai
Cc: mingo, tj, dchinner, fengguang.wu, jack, linux-kernel, Peter Zijlstra
On Fri, 12 Jul 2019 12:14:47 -0400
Qian Cai <cai@lca.pw> 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 <cai@lca.pw>
> ---
> 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.
-- 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings
2019-07-16 21:03 ` Steven Rostedt
@ 2019-07-18 20:11 ` Qian Cai
2019-07-29 10:04 ` Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: Qian Cai @ 2019-07-18 20:11 UTC (permalink / raw)
To: Steven Rostedt
Cc: mingo, tj, dchinner, fengguang.wu, jack, LKML, Peter Zijlstra,
Jens Axboe
> On Jul 16, 2019, at 5:03 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 12 Jul 2019 12:14:47 -0400
> Qian Cai <cai@lca.pw> 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 <cai@lca.pw>
>> ---
>> 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/1562948087-5374-1-git-send-email-cai@lca.pw/
>
> -- 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings
2019-07-16 21:03 ` Steven Rostedt
2019-07-18 20:11 ` Qian Cai
@ 2019-07-29 10:04 ` Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: Jan Kara @ 2019-07-29 10:04 UTC (permalink / raw)
To: Steven Rostedt
Cc: Qian Cai, mingo, tj, dchinner, fengguang.wu, jack, linux-kernel,
Peter Zijlstra
On Tue 16-07-19 17:03:39, Steven Rostedt wrote:
> On Fri, 12 Jul 2019 12:14:47 -0400
> Qian Cai <cai@lca.pw> 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 <cai@lca.pw>
> > ---
> > 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.
Yeah, I'd prefer to use strscpy_pad() for the copying as I'd be calmer if
we don't leak uninitialized memory to userspace. Thanks!
Honza
>
> -- 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;
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-29 10:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 16:14 [PATCH] trace/writeback: fix Wstringop-truncation warnings Qian Cai
2019-07-16 21:03 ` Steven Rostedt
2019-07-18 20:11 ` Qian Cai
2019-07-29 10:04 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).