[PATCH] bfs: prevent underflow in bfs_find_entry()

[PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

We check if "namelen" is larger than BFS_NAMELEN but we don't check
if it's less than zero so it causes a static checker.

    fs/bfs/dir.c:346 bfs_find_entry() warn: no lower bound on 'namelen'

It's nicer to make it unsigned anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/bfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index d8dfe3a0cb39..46a2663e5eb2 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -326,7 +326,7 @@ static struct buffer_head *bfs_find_entry(struct inode *dir,
 	struct buffer_head *bh = NULL;
 	struct bfs_dirent *de;
 	const unsigned char *name = child->name;
-	int namelen = child->len;
+	unsigned int namelen = child->len;
 
 	*res_dir = NULL;
 	if (namelen > BFS_NAMELEN)
-- 
2.11.0

[PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

We check if "namelen" is larger than BFS_NAMELEN but we don't check
if it's less than zero so it causes a static checker.

    fs/bfs/dir.c:346 bfs_find_entry() warn: no lower bound on 'namelen'

It's nicer to make it unsigned anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/bfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index d8dfe3a0cb39..46a2663e5eb2 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -326,7 +326,7 @@ static struct buffer_head *bfs_find_entry(struct inode *dir,
 	struct buffer_head *bh = NULL;
 	struct bfs_dirent *de;
 	const unsigned char *name = child->name;
-	int namelen = child->len;
+	unsigned int namelen = child->len;
 
 	*res_dir = NULL;
 	if (namelen > BFS_NAMELEN)
-- 
2.11.0

AW: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Walter Harms]

________________________________________
Von: kernel-janitors-owner@vger.kernel.org <kernel-janitors-owner@vger.kernel.org> im Auftrag von Dan Carpenter <dan.carpenter@oracle.com>
Gesendet: Samstag, 7. März 2020 07:08
An: Tigran A. Aivazian
Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: [PATCH] bfs: prevent underflow in bfs_find_entry()

We check if "namelen" is larger than BFS_NAMELEN but we don't check
if it's less than zero so it causes a static checker.

    fs/bfs/dir.c:346 bfs_find_entry() warn: no lower bound on 'namelen'

It's nicer to make it unsigned anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/bfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index d8dfe3a0cb39..46a2663e5eb2 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -326,7 +326,7 @@ static struct buffer_head *bfs_find_entry(struct inode *dir,
        struct buffer_head *bh = NULL;
        struct bfs_dirent *de;
        const unsigned char *name = child->name;
-       int namelen = child->len;
+       unsigned int namelen = child->len;

        *res_dir = NULL;
        if (namelen > BFS_NAMELEN)

hi Dan,
the namelen usage is fishy. It goes into bfs_namecmp()
where it is checked for namelen < BFS_NAMELEN, leaving
only the case ==.
bfs_namecmp() expects an int, so i would expect a warning.
Perhaps in this case it is better to change the if() into

if ( namelen <= 0 ||  namelen >= BFS_NAMELEN)
 return NULL;

note:  bfs_add_entry has the same "issue"

jm2c,
re,
 wh

--
2.11.0

AW: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Walter Harms]

________________________________________
Von: kernel-janitors-owner@vger.kernel.org <kernel-janitors-owner@vger.kernel.org> im Auftrag von Dan Carpenter <dan.carpenter@oracle.com>
Gesendet: Samstag, 7. März 2020 07:08
An: Tigran A. Aivazian
Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: [PATCH] bfs: prevent underflow in bfs_find_entry()

We check if "namelen" is larger than BFS_NAMELEN but we don't check
if it's less than zero so it causes a static checker.

    fs/bfs/dir.c:346 bfs_find_entry() warn: no lower bound on 'namelen'

It's nicer to make it unsigned anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/bfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index d8dfe3a0cb39..46a2663e5eb2 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -326,7 +326,7 @@ static struct buffer_head *bfs_find_entry(struct inode *dir,
        struct buffer_head *bh = NULL;
        struct bfs_dirent *de;
        const unsigned char *name = child->name;
-       int namelen = child->len;
+       unsigned int namelen = child->len;

        *res_dir = NULL;
        if (namelen > BFS_NAMELEN)

hi Dan,
the namelen usage is fishy. It goes into bfs_namecmp()
where it is checked for namelen < BFS_NAMELEN, leaving
only the case ==.
bfs_namecmp() expects an int, so i would expect a warning.
Perhaps in this case it is better to change the if() into

if ( namelen <= 0 ||  namelen >= BFS_NAMELEN)
 return NULL;

note:  bfs_add_entry has the same "issue"

jm2c,
re,
 wh

--
2.11.0

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

On Mon, Mar 09, 2020 at 08:40:28AM +0000, Walter Harms wrote:
> hi Dan,
> the namelen usage is fishy. It goes into bfs_namecmp()
> where it is checked for namelen < BFS_NAMELEN, leaving
> only the case =.

The rule in bfs_namecmp() is that the name has to be NUL terminated if
there is enough space.

regards,
dan carpenter

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

On Mon, Mar 09, 2020 at 08:40:28AM +0000, Walter Harms wrote:
> hi Dan,
> the namelen usage is fishy. It goes into bfs_namecmp()
> where it is checked for namelen < BFS_NAMELEN, leaving
> only the case ==.

The rule in bfs_namecmp() is that the name has to be NUL terminated if
there is enough space.

regards,
dan carpenter

AW: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Walter Harms]

________________________________________
Von: Dan Carpenter <dan.carpenter@oracle.com>
Gesendet: Dienstag, 10. März 2020 10:06
An: Walter Harms
Cc: Tigran A. Aivazian; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

On Mon, Mar 09, 2020 at 08:40:28AM +0000, Walter Harms wrote:
> hi Dan,
> the namelen usage is fishy. It goes into bfs_namecmp()
> where it is checked for namelen < BFS_NAMELEN, leaving
> only the case ==.

The rule in bfs_namecmp() is that the name has to be NUL terminated if
there is enough space.

that raises the question why is there a len paramter in the first place.
Surely the writer can make sure that there is always a NUL terminated
string, then it would be possible the use a simple strcmp() and the
range check is useless and can be removed.

seems a question for the maintainer.

re,
 wh

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Tigran Aivazian]

Hello,

On Tue, 10 Mar 2020 at 17:57, Walter Harms <wharms@bfs.de> wrote:
> that raises the question why is there a len paramter in the first place.
> Surely the writer can make sure that there is always a NUL terminated
> string, then it would be possible the use a simple strcmp() and the
> range check is useless and can be removed.
>
> seems a question for the maintainer.

Please have a look at, for example,
fs/ufs/dir.c:ufs_find_entry()/ufs_match() functions --- they do almost
the same thing as the ones in bfs. And, presumably, the line "int
namelen = qstr->len;" in ufs_find_entry() is causing the static
checker warning too, just like the one in bfs which Dan mentioned and
fixed. So, let's not over-complicate things (or make a mountain out of
a molehill) and accept Dan's patch as is.

Kind regards,
Tigran

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Tigran Aivazian]

Hello Dan,

On Sat, 7 Mar 2020 at 06:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> -       int namelen = child->len;
> +       unsigned int namelen = child->len;

Thank you, that is sensible, but have you actually verified that
attempting a lookup of a filename longer than 2.2 billion bytes causes
a problem? If that's the case, then your patch should be considered.
If not, it would seem to be a waste of time to worry about something
that cannot ever happen.

Kind regards,
Tigran

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Tigran Aivazian]

Hello Dan,

On Sat, 7 Mar 2020 at 06:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> -       int namelen = child->len;
> +       unsigned int namelen = child->len;

Thank you, that is sensible, but have you actually verified that
attempting a lookup of a filename longer than 2.2 billion bytes causes
a problem? If that's the case, then your patch should be considered.
If not, it would seem to be a waste of time to worry about something
that cannot ever happen.

Kind regards,
Tigran

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

On Mon, Mar 09, 2020 at 09:14:27AM +0000, Tigran Aivazian wrote:
> Hello Dan,
> 
> On Sat, 7 Mar 2020 at 06:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > -       int namelen = child->len;
> > +       unsigned int namelen = child->len;
> 
> Thank you, that is sensible, but have you actually verified that
> attempting a lookup of a filename longer than 2.2 billion bytes causes
> a problem? If that's the case, then your patch should be considered.
> If not, it would seem to be a waste of time to worry about something
> that cannot ever happen.

As the commit message says, this is just to silence a static checker
warning about checking for upper bounds but ignoring negatives.  The
check has found a number of problems in the past but it becomes less
useful if security reviewers have to sort through a bunch of false
positives.

regards,
dan carpenter

Re: [PATCH] bfs: prevent underflow in bfs_find_entry()

[Dan Carpenter]

On Mon, Mar 09, 2020 at 09:14:27AM +0000, Tigran Aivazian wrote:
> Hello Dan,
> 
> On Sat, 7 Mar 2020 at 06:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > -       int namelen = child->len;
> > +       unsigned int namelen = child->len;
> 
> Thank you, that is sensible, but have you actually verified that
> attempting a lookup of a filename longer than 2.2 billion bytes causes
> a problem? If that's the case, then your patch should be considered.
> If not, it would seem to be a waste of time to worry about something
> that cannot ever happen.

As the commit message says, this is just to silence a static checker
warning about checking for upper bounds but ignoring negatives.  The
check has found a number of problems in the past but it becomes less
useful if security reviewers have to sort through a bunch of false
positives.

regards,
dan carpenter