* [PATCH] gfs2: fix overread in the strlcpy of init_names
@ 2022-06-28 5:59 ` Dongliang Mu
0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-28 5:59 UTC (permalink / raw)
To: Bob Peterson, Andreas Gruenbacher, Steven Whitehouse, Jean Delvare
Cc: Dongliang Mu, syzkaller, cluster-devel, linux-kernel
From: Dongliang Mu <mudongliangabcd@gmail.com>
In init_names, strlcpy will overread the src string as the src string is
less than GFS2_FSNAME_LEN(256).
Fix this by modifying strlcpy back to snprintf, reverting
the commit 00377d8e3842.
Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
fs/gfs2/ops_fstype.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index c9b423c874a3..ee29b50d39b9 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
if (!table[0])
table = sdp->sd_vfs->s_id;
- strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
- strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
+ snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
+ snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
table = sdp->sd_table_name;
while ((table = strchr(table, '/')))
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: fix overread in the strlcpy of init_names
@ 2022-06-28 5:59 ` Dongliang Mu
0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-28 5:59 UTC (permalink / raw)
To: cluster-devel.redhat.com
From: Dongliang Mu <mudongliangabcd@gmail.com>
In init_names, strlcpy will overread the src string as the src string is
less than GFS2_FSNAME_LEN(256).
Fix this by modifying strlcpy back to snprintf, reverting
the commit 00377d8e3842.
Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
fs/gfs2/ops_fstype.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index c9b423c874a3..ee29b50d39b9 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
if (!table[0])
table = sdp->sd_vfs->s_id;
- strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
- strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
+ snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
+ snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
table = sdp->sd_table_name;
while ((table = strchr(table, '/')))
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] gfs2: fix overread in the strlcpy of init_names
2022-06-28 5:59 ` [Cluster-devel] " Dongliang Mu
@ 2022-06-28 12:06 ` Andreas Gruenbacher
-1 siblings, 0 replies; 8+ messages in thread
From: Andreas Gruenbacher @ 2022-06-28 12:06 UTC (permalink / raw)
To: Dongliang Mu
Cc: Bob Peterson, Steven Whitehouse, Jean Delvare, Dongliang Mu,
syzkaller, cluster-devel, LKML
Dongliang Mu,
On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> From: Dongliang Mu <mudongliangabcd@gmail.com>
>
> In init_names, strlcpy will overread the src string as the src string is
> less than GFS2_FSNAME_LEN(256).
>
> Fix this by modifying strlcpy back to snprintf, reverting
> the commit 00377d8e3842.
... if the source string isn't NULL-terminated. But in that case, the
code will still do the same thing with this patch. In other words,
this doesn't fix anything. So let's check for NULL termination
instead.
Thanks,
Andreas
> Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> fs/gfs2/ops_fstype.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c9b423c874a3..ee29b50d39b9 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> if (!table[0])
> table = sdp->sd_vfs->s_id;
>
> - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
>
> table = sdp->sd_table_name;
> while ((table = strchr(table, '/')))
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: fix overread in the strlcpy of init_names
@ 2022-06-28 12:06 ` Andreas Gruenbacher
0 siblings, 0 replies; 8+ messages in thread
From: Andreas Gruenbacher @ 2022-06-28 12:06 UTC (permalink / raw)
To: cluster-devel.redhat.com
Dongliang Mu,
On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> From: Dongliang Mu <mudongliangabcd@gmail.com>
>
> In init_names, strlcpy will overread the src string as the src string is
> less than GFS2_FSNAME_LEN(256).
>
> Fix this by modifying strlcpy back to snprintf, reverting
> the commit 00377d8e3842.
... if the source string isn't NULL-terminated. But in that case, the
code will still do the same thing with this patch. In other words,
this doesn't fix anything. So let's check for NULL termination
instead.
Thanks,
Andreas
> Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> fs/gfs2/ops_fstype.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c9b423c874a3..ee29b50d39b9 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> if (!table[0])
> table = sdp->sd_vfs->s_id;
>
> - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
>
> table = sdp->sd_table_name;
> while ((table = strchr(table, '/')))
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gfs2: fix overread in the strlcpy of init_names
2022-06-28 12:06 ` [Cluster-devel] " Andreas Gruenbacher
@ 2022-06-29 1:33 ` Dongliang Mu
-1 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-29 1:33 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Dongliang Mu, Bob Peterson, Steven Whitehouse, Jean Delvare,
syzkaller, cluster-devel, LKML
On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> Dongliang Mu,
>
> On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > In init_names, strlcpy will overread the src string as the src string is
> > less than GFS2_FSNAME_LEN(256).
> >
> > Fix this by modifying strlcpy back to snprintf, reverting
> > the commit 00377d8e3842.
>
> ... if the source string isn't NULL-terminated. But in that case, the
> code will still do the same thing with this patch. In other words,
> this doesn't fix anything. So let's check for NULL termination
> instead.
Partially yes. Even if the source string is NULL-terminated, strlcpy
will invoke memcpy to overread the adjacent memory of source string as
the specified length is longer than source string.
>
> Thanks,
> Andreas
>
> > Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> > Reported-by: syzkaller <syzkaller@googlegroups.com>
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > fs/gfs2/ops_fstype.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > index c9b423c874a3..ee29b50d39b9 100644
> > --- a/fs/gfs2/ops_fstype.c
> > +++ b/fs/gfs2/ops_fstype.c
> > @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> > if (!table[0])
> > table = sdp->sd_vfs->s_id;
> >
> > - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> > - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> > + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> > + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
> >
> > table = sdp->sd_table_name;
> > while ((table = strchr(table, '/')))
> > --
> > 2.35.1
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: fix overread in the strlcpy of init_names
@ 2022-06-29 1:33 ` Dongliang Mu
0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-29 1:33 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> Dongliang Mu,
>
> On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > In init_names, strlcpy will overread the src string as the src string is
> > less than GFS2_FSNAME_LEN(256).
> >
> > Fix this by modifying strlcpy back to snprintf, reverting
> > the commit 00377d8e3842.
>
> ... if the source string isn't NULL-terminated. But in that case, the
> code will still do the same thing with this patch. In other words,
> this doesn't fix anything. So let's check for NULL termination
> instead.
Partially yes. Even if the source string is NULL-terminated, strlcpy
will invoke memcpy to overread the adjacent memory of source string as
the specified length is longer than source string.
>
> Thanks,
> Andreas
>
> > Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> > Reported-by: syzkaller <syzkaller@googlegroups.com>
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > fs/gfs2/ops_fstype.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > index c9b423c874a3..ee29b50d39b9 100644
> > --- a/fs/gfs2/ops_fstype.c
> > +++ b/fs/gfs2/ops_fstype.c
> > @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> > if (!table[0])
> > table = sdp->sd_vfs->s_id;
> >
> > - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> > - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> > + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> > + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
> >
> > table = sdp->sd_table_name;
> > while ((table = strchr(table, '/')))
> > --
> > 2.35.1
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gfs2: fix overread in the strlcpy of init_names
2022-06-29 1:33 ` [Cluster-devel] " Dongliang Mu
@ 2022-06-29 1:56 ` Dongliang Mu
-1 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-29 1:56 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Dongliang Mu, Bob Peterson, Steven Whitehouse, Jean Delvare,
syzkaller, cluster-devel, LKML
On Wed, Jun 29, 2022 at 9:33 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> >
> > Dongliang Mu,
> >
> > On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > >
> > > In init_names, strlcpy will overread the src string as the src string is
> > > less than GFS2_FSNAME_LEN(256).
> > >
> > > Fix this by modifying strlcpy back to snprintf, reverting
> > > the commit 00377d8e3842.
> >
> > ... if the source string isn't NULL-terminated. But in that case, the
> > code will still do the same thing with this patch. In other words,
> > this doesn't fix anything. So let's check for NULL termination
> > instead.
>
> Partially yes. Even if the source string is NULL-terminated, strlcpy
> will invoke memcpy to overread the adjacent memory of source string as
> the specified length is longer than source string.
The above statement is incorrect after I double check the
implementation of strlcpy.
The correct fix should be NULL-termination check of src string.
>
> >
> > Thanks,
> > Andreas
> >
> > > Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> > > Reported-by: syzkaller <syzkaller@googlegroups.com>
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > > fs/gfs2/ops_fstype.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > index c9b423c874a3..ee29b50d39b9 100644
> > > --- a/fs/gfs2/ops_fstype.c
> > > +++ b/fs/gfs2/ops_fstype.c
> > > @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> > > if (!table[0])
> > > table = sdp->sd_vfs->s_id;
> > >
> > > - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> > > - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> > > + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> > > + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
> > >
> > > table = sdp->sd_table_name;
> > > while ((table = strchr(table, '/')))
> > > --
> > > 2.35.1
> > >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: fix overread in the strlcpy of init_names
@ 2022-06-29 1:56 ` Dongliang Mu
0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2022-06-29 1:56 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Wed, Jun 29, 2022 at 9:33 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> >
> > Dongliang Mu,
> >
> > On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > >
> > > In init_names, strlcpy will overread the src string as the src string is
> > > less than GFS2_FSNAME_LEN(256).
> > >
> > > Fix this by modifying strlcpy back to snprintf, reverting
> > > the commit 00377d8e3842.
> >
> > ... if the source string isn't NULL-terminated. But in that case, the
> > code will still do the same thing with this patch. In other words,
> > this doesn't fix anything. So let's check for NULL termination
> > instead.
>
> Partially yes. Even if the source string is NULL-terminated, strlcpy
> will invoke memcpy to overread the adjacent memory of source string as
> the specified length is longer than source string.
The above statement is incorrect after I double check the
implementation of strlcpy.
The correct fix should be NULL-termination check of src string.
>
> >
> > Thanks,
> > Andreas
> >
> > > Fixes: 00377d8e3842 ("[GFS2] Prefer strlcpy() over snprintf()")
> > > Reported-by: syzkaller <syzkaller@googlegroups.com>
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > > fs/gfs2/ops_fstype.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > index c9b423c874a3..ee29b50d39b9 100644
> > > --- a/fs/gfs2/ops_fstype.c
> > > +++ b/fs/gfs2/ops_fstype.c
> > > @@ -383,8 +383,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
> > > if (!table[0])
> > > table = sdp->sd_vfs->s_id;
> > >
> > > - strlcpy(sdp->sd_proto_name, proto, GFS2_FSNAME_LEN);
> > > - strlcpy(sdp->sd_table_name, table, GFS2_FSNAME_LEN);
> > > + snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
> > > + snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
> > >
> > > table = sdp->sd_table_name;
> > > while ((table = strchr(table, '/')))
> > > --
> > > 2.35.1
> > >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-29 1:57 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28 5:59 [PATCH] gfs2: fix overread in the strlcpy of init_names Dongliang Mu
2022-06-28 5:59 ` [Cluster-devel] " Dongliang Mu
2022-06-28 12:06 ` Andreas Gruenbacher
2022-06-28 12:06 ` [Cluster-devel] " Andreas Gruenbacher
2022-06-29 1:33 ` Dongliang Mu
2022-06-29 1:33 ` [Cluster-devel] " Dongliang Mu
2022-06-29 1:56 ` Dongliang Mu
2022-06-29 1:56 ` [Cluster-devel] " Dongliang Mu
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.