gfs2: fix overread in the strlcpy of init_names

[PATCH] gfs2: fix overread in the strlcpy of init_names

Posted by Dongliang Mu 3 years, 9 months ago

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

Re: [PATCH] gfs2: fix overread in the strlcpy of init_names

Posted by Andreas Gruenbacher 3 years, 9 months ago

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
>

Re: [PATCH] gfs2: fix overread in the strlcpy of init_names

Posted by Dongliang Mu 3 years, 9 months ago

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
> >
>

Re: [PATCH] gfs2: fix overread in the strlcpy of init_names

Posted by Dongliang Mu 3 years, 9 months ago

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
> > >
> >