2022-06-28 06:14:03

by Dongliang Mu

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

From: Dongliang Mu <[email protected]>

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 <[email protected]>
Signed-off-by: Dongliang Mu <[email protected]>
---
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

2022-06-28 12:44:59

by Andreas Gruenbacher

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

Dongliang Mu,

On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <[email protected]> wrote:
> From: Dongliang Mu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Dongliang Mu <[email protected]>
> ---
> 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
>

2022-06-29 02:05:44

by Dongliang Mu

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

On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <[email protected]> wrote:
>
> Dongliang Mu,
>
> On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <[email protected]> wrote:
> > From: Dongliang Mu <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Dongliang Mu <[email protected]>
> > ---
> > 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
> >
>

2022-06-29 02:11:42

by Dongliang Mu

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

On Wed, Jun 29, 2022 at 9:33 AM Dongliang Mu <[email protected]> wrote:
>
> On Tue, Jun 28, 2022 at 8:06 PM Andreas Gruenbacher <[email protected]> wrote:
> >
> > Dongliang Mu,
> >
> > On Tue, Jun 28, 2022 at 8:10 AM Dongliang Mu <[email protected]> wrote:
> > > From: Dongliang Mu <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Dongliang Mu <[email protected]>
> > > ---
> > > 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
> > >
> >