[PATCH 1/2] depmod: do not allow partial matches with "search" directive

[Anssi Hannula]

Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.

Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}

[Anssi Hannula]

---
tools/depmod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 328e578..37e6afd 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1167,7 +1167,7 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
oldprio = bprio;

DBG("priorities: built-in: %d, old: %d, new: %d\n",
- bprio, newprio, oldprio);
+ bprio, oldprio, newprio);

return newprio <= oldprio;
}

[Lucas De Marchi]

Hi Anssi,

Post by Anssi Hannula
Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.
Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}
--

Both patches have been applied. Thanks a lot.

I added some test cases for depmod. Could you take a look if they are ok?

--
Lucas De Marchi

[Anssi Hannula]

Post by Lucas De Marchi
Hi Anssi,

Hi,

Post by Lucas De Marchi

Post by Anssi Hannula
Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.
Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}
--

Both patches have been applied. Thanks a lot.
I added some test cases for depmod. Could you take a look if they are ok?

Well, just reversing the "search" directive does not ensure the bug is
caught by the test (and indeed it is not on my testing, i.e. tests still
pass after reverting my fix locally), since it does not alter the
initial order the modules are found.

To hit the bug, the code needs to hit the short-directory module first,
so that the short path is in "oldpath" and the long path is in "newpath".

[Lucas De Marchi]

Post by Anssi Hannula

Post by Lucas De Marchi
Hi Anssi,

Hi,

Post by Lucas De Marchi

Post by Anssi Hannula
Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.
Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}
--

Both patches have been applied. Thanks a lot.
I added some test cases for depmod. Could you take a look if they are ok?

Well, just reversing the "search" directive does not ensure the bug is
caught by the test (and indeed it is not on my testing, i.e. tests still
pass after reverting my fix locally), since it does not alter the
initial order the modules are found.

hum... assuming the traverse order is stable, one of them should hit
it. In fact. Reverting your commit does reproduce the bug for me 100%
of the time.

Post by Anssi Hannula
To hit the bug, the code needs to hit the short-directory module first,
so that the short path is in "oldpath" and the long path is in "newpath".

But it really depends if the short is the right one as opposed to the
long one. Otherwise it would be correct to replace the module, even
if by accident.

Are you testing this with "make check" or running locally on your system?

[Anssi Hannula]

Post by Lucas De Marchi

Post by Anssi Hannula

Post by Lucas De Marchi
Hi Anssi,

Hi,

Post by Lucas De Marchi

Post by Anssi Hannula
Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.
Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}
--

Both patches have been applied. Thanks a lot.
I added some test cases for depmod. Could you take a look if they are ok?

Well, just reversing the "search" directive does not ensure the bug is
caught by the test (and indeed it is not on my testing, i.e. tests still
pass after reverting my fix locally), since it does not alter the
initial order the modules are found.

hum... assuming the traverse order is stable, one of them should hit
it. In fact. Reverting your commit does reproduce the bug for me 100%
of the time.

Post by Anssi Hannula
To hit the bug, the code needs to hit the short-directory module first,
so that the short path is in "oldpath" and the long path is in "newpath".

But it really depends if the short is the right one as opposed to the
long one. Otherwise it would be correct to replace the module, even
if by accident.

I don't think so. Let me try to show what I mean with an example with
all the 4 combinations with unfixed code:

Let's have foo/module.ko and foobar/module.ko.

Traverse order foo,foobar:
oldpath = foo/module.ko
newpath = foobar/module.ko

"search foo(2) foobar(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: newprio = 2
oldprio fallback: oldprio = 0
=> oldprio 0, newprio 2
=> WRONG

"search foobar(2) foo(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: newprio = 2
oldprio fallback: oldprio = 0
=> oldprio 0, newprio 2
=> OK


Traverse order foobar,foo:
oldpath = foobar/module.ko
newpath = foo/module.ko

"search foo(2) foobar(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: oldprio = 1
iteration 2: newprio = 2
=> oldprio 1, newprio 2
=> OK

"search foobar(2) foo(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: oldprio = 2
=> oldprio 2, newprio 1
=> OK


So the bug is triggered only if the shorter name is higher-prio _and_
shorter name is traversed first. If the long name is traversed first,
the bug don't trigger with either "search" directive order (and on my
"make check" runs this is the case).

Post by Lucas De Marchi
Are you testing this with "make check" or running locally on your system?

make check.

[Lucas De Marchi]

Post by Anssi Hannula

Post by Lucas De Marchi

Post by Anssi Hannula

Post by Lucas De Marchi
Hi Anssi,

Hi,

Post by Lucas De Marchi

Post by Anssi Hannula
Currently e.g. "search foo foobar built-in" will cause unpredictable
results if baz.ko is in both foo/ and foobar/, since "foo" in search may
match both of those directories and the preferred module therefore
depends on processing order.
Fix the code to ensure that the match is performed on full pathname
components only.
---
tools/depmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/depmod.c b/tools/depmod.c
index 9f83ee8..328e578 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
DBG("search %s\n", se->builtin ? "built-in" : se->path);
if (se->builtin)
bprio = i;
- else if (newlen >= se->len &&
+ else if (newlen > se->len && newpath[se->len] == '/' &&
memcmp(se->path, newpath, se->len) == 0)
newprio = i;
- else if (oldlen >= se->len &&
+ else if (oldlen > se->len && oldpath[se->len] == '/' &&
memcmp(se->path, oldpath, se->len) == 0)
oldprio = i;
}
--

Both patches have been applied. Thanks a lot.
I added some test cases for depmod. Could you take a look if they are ok?

Well, just reversing the "search" directive does not ensure the bug is
caught by the test (and indeed it is not on my testing, i.e. tests still
pass after reverting my fix locally), since it does not alter the
initial order the modules are found.

hum... assuming the traverse order is stable, one of them should hit
it. In fact. Reverting your commit does reproduce the bug for me 100%
of the time.

Post by Anssi Hannula
To hit the bug, the code needs to hit the short-directory module first,
so that the short path is in "oldpath" and the long path is in "newpath".

But it really depends if the short is the right one as opposed to the
long one. Otherwise it would be correct to replace the module, even
if by accident.

I don't think so. Let me try to show what I mean with an example with
Let's have foo/module.ko and foobar/module.ko.
oldpath = foo/module.ko
newpath = foobar/module.ko
"search foo(2) foobar(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: newprio = 2
oldprio fallback: oldprio = 0
=> oldprio 0, newprio 2
=> WRONG
"search foobar(2) foo(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: newprio = 2
oldprio fallback: oldprio = 0
=> oldprio 0, newprio 2
=> OK
oldpath = foobar/module.ko
newpath = foo/module.ko
"search foo(2) foobar(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: oldprio = 1
iteration 2: newprio = 2
=> oldprio 1, newprio 2
=> OK
"search foobar(2) foo(1) built-in(0)"
iteration 0: bprio = 0
iteration 1: newprio = 1
iteration 2: oldprio = 2
=> oldprio 2, newprio 1
=> OK
So the bug is triggered only if the shorter name is higher-prio _and_
shorter name is traversed first. If the long name is traversed first,
the bug don't trigger with either "search" directive order (and on my
"make check" runs this is the case).

Yep, you are right. And I'm not sure there's a way to trigger the bug
to always reproduce :(
If I don't figure out a way to do this I think I'll just revert the
test, or at least remove the double test.

thanks

Lucas De Marchi.