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 <[email protected]>
---
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
________________________________________
Von: [email protected] <[email protected]> im Auftrag von Dan Carpenter <[email protected]>
Gesendet: Samstag, 7. M?rz 2020 07:08
An: Tigran A. Aivazian
Cc: [email protected]; [email protected]
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 <[email protected]>
---
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
Hello Dan,
On Sat, 7 Mar 2020 at 06:08, Dan Carpenter <[email protected]> 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
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 <[email protected]> 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
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
________________________________________
Von: Dan Carpenter <[email protected]>
Gesendet: Dienstag, 10. M?rz 2020 10:06
An: Walter Harms
Cc: Tigran A. Aivazian; [email protected]; [email protected]
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
Hello,
On Tue, 10 Mar 2020 at 17:57, Walter Harms <[email protected]> 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