* [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls
@ 2019-01-18 19:32 Mathieu Malaterre
2019-01-25 4:26 ` Nick Desaulniers
0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Malaterre @ 2019-01-18 19:32 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Malaterre, Nick Desaulniers, Ingo Molnar, linux-kernel
In the past an attempt was made to remove a set of warnings triggered by
gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
rejected as one of the desired behavior is to keep initializing the rest
of the destination string whenever the source string is less than the
size.
However the code makes it clear that this is not a desired behavior to
copy the entire 32 characters into the destination buffer, since it is
expected that the string is NUL terminated. So change the maximum number
of characters to be copied to be the size of the destination buffer
minus 1.
Break long lines and make this patch go through `checkpatch --strict` with
no errors.
This commit removes the following warnings:
include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
Link: https://lore.kernel.org/patchwork/patch/910830/
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index 32db72c7c055..7bc58980f84f 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
TP_fast_assign(
strncpy(__entry->name,
- mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
+ mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
+ "(unknown)", sizeof(__entry->name) - 1);
__entry->ino = mapping ? mapping->host->i_ino : 0;
__entry->index = page->index;
),
@@ -97,7 +98,8 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
/* may be called for files on pseudo FSes w/ unregistered bdi */
strncpy(__entry->name,
- bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
+ bdi->dev ? dev_name(bdi->dev) : "(unknown)",
+ sizeof(__entry->name) - 1);
__entry->ino = inode->i_ino;
__entry->state = inode->i_state;
__entry->flags = flags;
@@ -177,7 +179,8 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
TP_fast_assign(
strncpy(__entry->name,
- dev_name(inode_to_bdi(inode)->dev), 32);
+ dev_name(inode_to_bdi(inode)->dev),
+ sizeof(__entry->name) - 1);
__entry->ino = inode->i_ino;
__entry->sync_mode = wbc->sync_mode;
__entry->cgroup_ino = __trace_wbc_assign_cgroup(wbc);
@@ -221,7 +224,8 @@ DECLARE_EVENT_CLASS(writeback_work_class,
),
TP_fast_assign(
strncpy(__entry->name,
- wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", 32);
+ wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)",
+ sizeof(__entry->name) - 1);
__entry->nr_pages = work->nr_pages;
__entry->sb_dev = work->sb ? work->sb->s_dev : 0;
__entry->sync_mode = work->sync_mode;
@@ -274,7 +278,8 @@ DECLARE_EVENT_CLASS(writeback_class,
__field(unsigned int, cgroup_ino)
),
TP_fast_assign(
- strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
+ strncpy(__entry->name, dev_name(wb->bdi->dev),
+ sizeof(__entry->name) - 1);
__entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
),
TP_printk("bdi %s: cgroup_ino=%u",
@@ -296,7 +301,8 @@ TRACE_EVENT(writeback_bdi_register,
__array(char, name, 32)
),
TP_fast_assign(
- strncpy(__entry->name, dev_name(bdi->dev), 32);
+ strncpy(__entry->name, dev_name(bdi->dev),
+ sizeof(__entry->name) - 1);
),
TP_printk("bdi %s",
__entry->name
@@ -321,7 +327,8 @@ DECLARE_EVENT_CLASS(wbc_class,
),
TP_fast_assign(
- strncpy(__entry->name, dev_name(bdi->dev), 32);
+ strncpy(__entry->name, dev_name(bdi->dev),
+ sizeof(__entry->name) - 1);
__entry->nr_to_write = wbc->nr_to_write;
__entry->pages_skipped = wbc->pages_skipped;
__entry->sync_mode = wbc->sync_mode;
@@ -372,7 +379,8 @@ TRACE_EVENT(writeback_queue_io,
),
TP_fast_assign(
unsigned long *older_than_this = work->older_than_this;
- strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
+ strncpy(__entry->name, dev_name(wb->bdi->dev),
+ sizeof(__entry->name) - 1);
__entry->older = older_than_this ? *older_than_this : 0;
__entry->age = older_than_this ?
(jiffies - *older_than_this) * 1000 / HZ : -1;
@@ -584,7 +592,8 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
TP_fast_assign(
strncpy(__entry->name,
- dev_name(inode_to_bdi(inode)->dev), 32);
+ dev_name(inode_to_bdi(inode)->dev),
+ sizeof(__entry->name) - 1);
__entry->ino = inode->i_ino;
__entry->state = inode->i_state;
__entry->dirtied_when = inode->dirtied_when;
@@ -658,7 +667,8 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
TP_fast_assign(
strncpy(__entry->name,
- dev_name(inode_to_bdi(inode)->dev), 32);
+ dev_name(inode_to_bdi(inode)->dev),
+ sizeof(__entry->name) - 1);
__entry->ino = inode->i_ino;
__entry->state = inode->i_state;
__entry->dirtied_when = inode->dirtied_when;
--
2.19.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls
2019-01-18 19:32 [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls Mathieu Malaterre
@ 2019-01-25 4:26 ` Nick Desaulniers
2019-01-25 7:58 ` Mathieu Malaterre
0 siblings, 1 reply; 5+ messages in thread
From: Nick Desaulniers @ 2019-01-25 4:26 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: Steven Rostedt, Ingo Molnar, Linux Kernel Mailing List, Kees Cook
On Fri, Jan 18, 2019 at 11:32 AM Mathieu Malaterre <malat@debian.org> wrote:
>
> In the past an attempt was made to remove a set of warnings triggered by
> gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
> rejected as one of the desired behavior is to keep initializing the rest
> of the destination string whenever the source string is less than the
> size.
>
> However the code makes it clear that this is not a desired behavior to
> copy the entire 32 characters into the destination buffer, since it is
> expected that the string is NUL terminated. So change the maximum number
> of characters to be copied to be the size of the destination buffer
> minus 1.
>
> Break long lines and make this patch go through `checkpatch --strict` with
> no errors.
>
> This commit removes the following warnings:
>
> include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>
> Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> Link: https://lore.kernel.org/patchwork/patch/910830/
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> ---
> include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index 32db72c7c055..7bc58980f84f 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
>
> TP_fast_assign(
> strncpy(__entry->name,
> - mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
> + mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
> + "(unknown)", sizeof(__entry->name) - 1);
Does strncpy guarantee that destination will be NULL terminated if
(sizeof(src) > sizeof(dst)) || (32 > sizeof(dst))?
> __entry->ino = mapping ? mapping->host->i_ino : 0;
> __entry->index = page->index;
> ),
> @@ -97,7 +98,8 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
>
> /* may be called for files on pseudo FSes w/ unregistered bdi */
> strncpy(__entry->name,
> - bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
> + bdi->dev ? dev_name(bdi->dev) : "(unknown)",
> + sizeof(__entry->name) - 1);
> __entry->ino = inode->i_ino;
> __entry->state = inode->i_state;
> __entry->flags = flags;
> @@ -177,7 +179,8 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
>
> TP_fast_assign(
> strncpy(__entry->name,
> - dev_name(inode_to_bdi(inode)->dev), 32);
> + dev_name(inode_to_bdi(inode)->dev),
> + sizeof(__entry->name) - 1);
> __entry->ino = inode->i_ino;
> __entry->sync_mode = wbc->sync_mode;
> __entry->cgroup_ino = __trace_wbc_assign_cgroup(wbc);
> @@ -221,7 +224,8 @@ DECLARE_EVENT_CLASS(writeback_work_class,
> ),
> TP_fast_assign(
> strncpy(__entry->name,
> - wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", 32);
> + wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)",
> + sizeof(__entry->name) - 1);
> __entry->nr_pages = work->nr_pages;
> __entry->sb_dev = work->sb ? work->sb->s_dev : 0;
> __entry->sync_mode = work->sync_mode;
> @@ -274,7 +278,8 @@ DECLARE_EVENT_CLASS(writeback_class,
> __field(unsigned int, cgroup_ino)
> ),
> TP_fast_assign(
> - strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> + strncpy(__entry->name, dev_name(wb->bdi->dev),
> + sizeof(__entry->name) - 1);
> __entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
> ),
> TP_printk("bdi %s: cgroup_ino=%u",
> @@ -296,7 +301,8 @@ TRACE_EVENT(writeback_bdi_register,
> __array(char, name, 32)
> ),
> TP_fast_assign(
> - strncpy(__entry->name, dev_name(bdi->dev), 32);
> + strncpy(__entry->name, dev_name(bdi->dev),
> + sizeof(__entry->name) - 1);
> ),
> TP_printk("bdi %s",
> __entry->name
> @@ -321,7 +327,8 @@ DECLARE_EVENT_CLASS(wbc_class,
> ),
>
> TP_fast_assign(
> - strncpy(__entry->name, dev_name(bdi->dev), 32);
> + strncpy(__entry->name, dev_name(bdi->dev),
> + sizeof(__entry->name) - 1);
> __entry->nr_to_write = wbc->nr_to_write;
> __entry->pages_skipped = wbc->pages_skipped;
> __entry->sync_mode = wbc->sync_mode;
> @@ -372,7 +379,8 @@ TRACE_EVENT(writeback_queue_io,
> ),
> TP_fast_assign(
> unsigned long *older_than_this = work->older_than_this;
> - strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> + strncpy(__entry->name, dev_name(wb->bdi->dev),
> + sizeof(__entry->name) - 1);
> __entry->older = older_than_this ? *older_than_this : 0;
> __entry->age = older_than_this ?
> (jiffies - *older_than_this) * 1000 / HZ : -1;
> @@ -584,7 +592,8 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
>
> TP_fast_assign(
> strncpy(__entry->name,
> - dev_name(inode_to_bdi(inode)->dev), 32);
> + dev_name(inode_to_bdi(inode)->dev),
> + sizeof(__entry->name) - 1);
> __entry->ino = inode->i_ino;
> __entry->state = inode->i_state;
> __entry->dirtied_when = inode->dirtied_when;
> @@ -658,7 +667,8 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
>
> TP_fast_assign(
> strncpy(__entry->name,
> - dev_name(inode_to_bdi(inode)->dev), 32);
> + dev_name(inode_to_bdi(inode)->dev),
> + sizeof(__entry->name) - 1);
> __entry->ino = inode->i_ino;
> __entry->state = inode->i_state;
> __entry->dirtied_when = inode->dirtied_when;
> --
> 2.19.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls
2019-01-25 4:26 ` Nick Desaulniers
@ 2019-01-25 7:58 ` Mathieu Malaterre
2019-01-25 17:06 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Malaterre @ 2019-01-25 7:58 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Steven Rostedt, Ingo Molnar, Linux Kernel Mailing List, Kees Cook
On Fri, Jan 25, 2019 at 5:26 AM Nick Desaulniers
<nick.desaulniers@gmail.com> wrote:
>
> On Fri, Jan 18, 2019 at 11:32 AM Mathieu Malaterre <malat@debian.org> wrote:
> >
> > In the past an attempt was made to remove a set of warnings triggered by
> > gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
> > rejected as one of the desired behavior is to keep initializing the rest
> > of the destination string whenever the source string is less than the
> > size.
> >
> > However the code makes it clear that this is not a desired behavior to
> > copy the entire 32 characters into the destination buffer, since it is
> > expected that the string is NUL terminated. So change the maximum number
> > of characters to be copied to be the size of the destination buffer
> > minus 1.
> >
> > Break long lines and make this patch go through `checkpatch --strict` with
> > no errors.
> >
> > This commit removes the following warnings:
> >
> > include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> >
> > Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> > Link: https://lore.kernel.org/patchwork/patch/910830/
> > Signed-off-by: Mathieu Malaterre <malat@debian.org>
> > ---
> > include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
> > 1 file changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> > index 32db72c7c055..7bc58980f84f 100644
> > --- a/include/trace/events/writeback.h
> > +++ b/include/trace/events/writeback.h
> > @@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
> >
> > TP_fast_assign(
> > strncpy(__entry->name,
> > - mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
> > + mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
> > + "(unknown)", sizeof(__entry->name) - 1);
>
> Does strncpy guarantee that destination will be NULL terminated if
> (sizeof(src) > sizeof(dst)) || (32 > sizeof(dst))?
No, that's the point here:
https://www.kernel.org/doc/htmldocs/kernel-api/API-strncpy.html
...
The result is not NUL-terminated if the source exceeds count bytes.
In the case where the length of src is less than that of count, the
remainder of dest will be padded with NUL.
...
One should use strlcpy for the use case you describe:
https://www.kernel.org/doc/htmldocs/kernel-api/API-strlcpy.html
> > __entry->ino = mapping ? mapping->host->i_ino : 0;
> > __entry->index = page->index;
> > ),
> > @@ -97,7 +98,8 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
> >
> > /* may be called for files on pseudo FSes w/ unregistered bdi */
> > strncpy(__entry->name,
> > - bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
> > + bdi->dev ? dev_name(bdi->dev) : "(unknown)",
> > + sizeof(__entry->name) - 1);
> > __entry->ino = inode->i_ino;
> > __entry->state = inode->i_state;
> > __entry->flags = flags;
> > @@ -177,7 +179,8 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
> >
> > TP_fast_assign(
> > strncpy(__entry->name,
> > - dev_name(inode_to_bdi(inode)->dev), 32);
> > + dev_name(inode_to_bdi(inode)->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->ino = inode->i_ino;
> > __entry->sync_mode = wbc->sync_mode;
> > __entry->cgroup_ino = __trace_wbc_assign_cgroup(wbc);
> > @@ -221,7 +224,8 @@ DECLARE_EVENT_CLASS(writeback_work_class,
> > ),
> > TP_fast_assign(
> > strncpy(__entry->name,
> > - wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", 32);
> > + wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)",
> > + sizeof(__entry->name) - 1);
> > __entry->nr_pages = work->nr_pages;
> > __entry->sb_dev = work->sb ? work->sb->s_dev : 0;
> > __entry->sync_mode = work->sync_mode;
> > @@ -274,7 +278,8 @@ DECLARE_EVENT_CLASS(writeback_class,
> > __field(unsigned int, cgroup_ino)
> > ),
> > TP_fast_assign(
> > - strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> > + strncpy(__entry->name, dev_name(wb->bdi->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
> > ),
> > TP_printk("bdi %s: cgroup_ino=%u",
> > @@ -296,7 +301,8 @@ TRACE_EVENT(writeback_bdi_register,
> > __array(char, name, 32)
> > ),
> > TP_fast_assign(
> > - strncpy(__entry->name, dev_name(bdi->dev), 32);
> > + strncpy(__entry->name, dev_name(bdi->dev),
> > + sizeof(__entry->name) - 1);
> > ),
> > TP_printk("bdi %s",
> > __entry->name
> > @@ -321,7 +327,8 @@ DECLARE_EVENT_CLASS(wbc_class,
> > ),
> >
> > TP_fast_assign(
> > - strncpy(__entry->name, dev_name(bdi->dev), 32);
> > + strncpy(__entry->name, dev_name(bdi->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->nr_to_write = wbc->nr_to_write;
> > __entry->pages_skipped = wbc->pages_skipped;
> > __entry->sync_mode = wbc->sync_mode;
> > @@ -372,7 +379,8 @@ TRACE_EVENT(writeback_queue_io,
> > ),
> > TP_fast_assign(
> > unsigned long *older_than_this = work->older_than_this;
> > - strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> > + strncpy(__entry->name, dev_name(wb->bdi->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->older = older_than_this ? *older_than_this : 0;
> > __entry->age = older_than_this ?
> > (jiffies - *older_than_this) * 1000 / HZ : -1;
> > @@ -584,7 +592,8 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
> >
> > TP_fast_assign(
> > strncpy(__entry->name,
> > - dev_name(inode_to_bdi(inode)->dev), 32);
> > + dev_name(inode_to_bdi(inode)->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->ino = inode->i_ino;
> > __entry->state = inode->i_state;
> > __entry->dirtied_when = inode->dirtied_when;
> > @@ -658,7 +667,8 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
> >
> > TP_fast_assign(
> > strncpy(__entry->name,
> > - dev_name(inode_to_bdi(inode)->dev), 32);
> > + dev_name(inode_to_bdi(inode)->dev),
> > + sizeof(__entry->name) - 1);
> > __entry->ino = inode->i_ino;
> > __entry->state = inode->i_state;
> > __entry->dirtied_when = inode->dirtied_when;
> > --
> > 2.19.2
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls
2019-01-25 7:58 ` Mathieu Malaterre
@ 2019-01-25 17:06 ` Kees Cook
2019-01-31 10:58 ` Mathieu Malaterre
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2019-01-25 17:06 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: Nick Desaulniers, Steven Rostedt, Ingo Molnar, Linux Kernel Mailing List
On Fri, Jan 25, 2019 at 9:18 PM Mathieu Malaterre <malat@debian.org> wrote:
>
> On Fri, Jan 25, 2019 at 5:26 AM Nick Desaulniers
> <nick.desaulniers@gmail.com> wrote:
> >
> > On Fri, Jan 18, 2019 at 11:32 AM Mathieu Malaterre <malat@debian.org> wrote:
> > >
> > > In the past an attempt was made to remove a set of warnings triggered by
> > > gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
> > > rejected as one of the desired behavior is to keep initializing the rest
> > > of the destination string whenever the source string is less than the
> > > size.
> > >
> > > However the code makes it clear that this is not a desired behavior to
> > > copy the entire 32 characters into the destination buffer, since it is
> > > expected that the string is NUL terminated. So change the maximum number
> > > of characters to be copied to be the size of the destination buffer
> > > minus 1.
> > >
> > > Break long lines and make this patch go through `checkpatch --strict` with
> > > no errors.
> > >
> > > This commit removes the following warnings:
> > >
> > > include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > >
> > > Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > Link: https://lore.kernel.org/patchwork/patch/910830/
> > > Signed-off-by: Mathieu Malaterre <malat@debian.org>
> > > ---
> > > include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
> > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> > > index 32db72c7c055..7bc58980f84f 100644
> > > --- a/include/trace/events/writeback.h
> > > +++ b/include/trace/events/writeback.h
> > > @@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
> > >
> > > TP_fast_assign(
> > > strncpy(__entry->name,
> > > - mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
> > > + mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
> > > + "(unknown)", sizeof(__entry->name) - 1);
> >
> > Does strncpy guarantee that destination will be NULL terminated if
> > (sizeof(src) > sizeof(dst)) || (32 > sizeof(dst))?
>
> No, that's the point here:
>
> https://www.kernel.org/doc/htmldocs/kernel-api/API-strncpy.html
>
> ...
> The result is not NUL-terminated if the source exceeds count bytes.
>
> In the case where the length of src is less than that of count, the
> remainder of dest will be padded with NUL.
> ...
>
> One should use strlcpy for the use case you describe:
>
> https://www.kernel.org/doc/htmldocs/kernel-api/API-strlcpy.html
Please use strspy() -- strlcpy() will read the source beyond the length limit.
https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls
2019-01-25 17:06 ` Kees Cook
@ 2019-01-31 10:58 ` Mathieu Malaterre
0 siblings, 0 replies; 5+ messages in thread
From: Mathieu Malaterre @ 2019-01-31 10:58 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Steven Rostedt, Ingo Molnar, Kees Cook, Linux Kernel Mailing List
Nick,
On Fri, Jan 25, 2019 at 6:06 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Jan 25, 2019 at 9:18 PM Mathieu Malaterre <malat@debian.org> wrote:
> >
> > On Fri, Jan 25, 2019 at 5:26 AM Nick Desaulniers
> > <nick.desaulniers@gmail.com> wrote:
> > >
> > > On Fri, Jan 18, 2019 at 11:32 AM Mathieu Malaterre <malat@debian.org> wrote:
> > > >
> > > > In the past an attempt was made to remove a set of warnings triggered by
> > > > gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
> > > > rejected as one of the desired behavior is to keep initializing the rest
> > > > of the destination string whenever the source string is less than the
> > > > size.
> > > >
> > > > However the code makes it clear that this is not a desired behavior to
> > > > copy the entire 32 characters into the destination buffer, since it is
> > > > expected that the string is NUL terminated. So change the maximum number
> > > > of characters to be copied to be the size of the destination buffer
> > > > minus 1.
> > > >
> > > > Break long lines and make this patch go through `checkpatch --strict` with
> > > > no errors.
> > > >
> > > > This commit removes the following warnings:
> > > >
> > > > include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > > include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
> > > >
> > > > Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > > Link: https://lore.kernel.org/patchwork/patch/910830/
> > > > Signed-off-by: Mathieu Malaterre <malat@debian.org>
> > > > ---
> > > > include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
> > > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> > > > index 32db72c7c055..7bc58980f84f 100644
> > > > --- a/include/trace/events/writeback.h
> > > > +++ b/include/trace/events/writeback.h
> > > > @@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
> > > >
> > > > TP_fast_assign(
> > > > strncpy(__entry->name,
> > > > - mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
> > > > + mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
> > > > + "(unknown)", sizeof(__entry->name) - 1);
> > >
> > > Does strncpy guarantee that destination will be NULL terminated if
> > > (sizeof(src) > sizeof(dst)) || (32 > sizeof(dst))?
> >
> > No, that's the point here:
> >
> > https://www.kernel.org/doc/htmldocs/kernel-api/API-strncpy.html
> >
> > ...
> > The result is not NUL-terminated if the source exceeds count bytes.
> >
> > In the case where the length of src is less than that of count, the
> > remainder of dest will be padded with NUL.
> > ...
> >
> > One should use strlcpy for the use case you describe:
> >
> > https://www.kernel.org/doc/htmldocs/kernel-api/API-strlcpy.html
>
> Please use strspy() -- strlcpy() will read the source beyond the length limit.
> https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html
Quoting your previous attempt:
> Eric points out this wont initialize the rest of the dest if src if
> less than size.
Could you include 'Eric' in the thread ? I'd like to know his opinion
on a patch with strspy + memset or if strspy is enough here.
> --
> Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-31 10:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18 19:32 [PATCH] writeback: tracing: Copy only up to 31 characters in strncpy() calls Mathieu Malaterre
2019-01-25 4:26 ` Nick Desaulniers
2019-01-25 7:58 ` Mathieu Malaterre
2019-01-25 17:06 ` Kees Cook
2019-01-31 10:58 ` Mathieu Malaterre
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.