| Message ID | 20241029161341.14063-1-marc.c.dionne@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tools/mm: Fix slabinfo crash when MAX_SLABS is exceeded | expand |
On Tue, 29 Oct 2024 13:13:41 -0300 Marc Dionne <marc.c.dionne@gmail.com> wrote: > From: Marc Dionne <marc.dionne@auristor.com> > > The number of slabs can easily exceed the hard coded MAX_SLABS in the > slabinfo tool, causing it to overwrite memory and crash. > > Increase the value of MAX_SLABS, and check if that has been exceeded for > each new slab, instead of at the end when it's already too late. Also > move the check for MAX_ALIASES into the loop body. Thanks. > --- a/tools/mm/slabinfo.c > +++ b/tools/mm/slabinfo.c > @@ -21,7 +21,7 @@ > #include <regex.h> > #include <errno.h> > > -#define MAX_SLABS 500 > +#define MAX_SLABS 1000 That isn't a very large increase. > #define MAX_ALIASES 500 > #define MAX_NODES 1024 > > @@ -1240,6 +1240,8 @@ static void read_slab_dir(void) > p--; > alias->ref = strdup(p); > alias++; > + if (alias - aliasinfo > MAX_ALIASES) > + fatal("Too many aliases\n"); > break; > case DT_DIR: > if (chdir(de->d_name)) > @@ -1301,6 +1303,8 @@ static void read_slab_dir(void) > if (slab->name[0] == ':') > alias_targets++; > slab++; > + if (slab - slabinfo > MAX_SLABS) > + fatal("Too many slabs\n"); > break; This could be improved - if the number of slabs is exactly equal to MAX_SLABS we'll unnecessarily report an error. Wouldn't this alteration be better? --- a/tools/mm/slabinfo.c~tools-mm-fix-slabinfo-crash-when-max_slabs-is-exceeded-fix +++ a/tools/mm/slabinfo.c @@ -1228,6 +1228,8 @@ static void read_slab_dir(void) continue; switch (de->d_type) { case DT_LNK: + if (alias - aliasinfo >= MAX_ALIASES) + fatal("Too many aliases\n"); alias->name = strdup(de->d_name); count = readlink(de->d_name, buffer, sizeof(buffer)-1); @@ -1240,10 +1242,10 @@ static void read_slab_dir(void) p--; alias->ref = strdup(p); alias++; - if (alias - aliasinfo > MAX_ALIASES) - fatal("Too many aliases\n"); break; case DT_DIR: + if (slab - slabinfo >= MAX_SLABS) + fatal("Too many slabs\n"); if (chdir(de->d_name)) fatal("Unable to access slab %s\n", slab->name); slab->name = strdup(de->d_name); @@ -1305,8 +1307,6 @@ static void read_slab_dir(void) if (slab->name[0] == ':') alias_targets++; slab++; - if (slab - slabinfo > MAX_SLABS) - fatal("Too many slabs\n"); break; default : fatal("Unknown file type %lx\n", de->d_type);
On Tue, Oct 29, 2024 at 9:45 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 29 Oct 2024 13:13:41 -0300 Marc Dionne <marc.c.dionne@gmail.com> wrote: > > > From: Marc Dionne <marc.dionne@auristor.com> > > > > The number of slabs can easily exceed the hard coded MAX_SLABS in the > > slabinfo tool, causing it to overwrite memory and crash. > > > > Increase the value of MAX_SLABS, and check if that has been exceeded for > > each new slab, instead of at the end when it's already too late. Also > > move the check for MAX_ALIASES into the loop body. > > Thanks. > > > --- a/tools/mm/slabinfo.c > > +++ b/tools/mm/slabinfo.c > > @@ -21,7 +21,7 @@ > > #include <regex.h> > > #include <errno.h> > > > > -#define MAX_SLABS 500 > > +#define MAX_SLABS 1000 > > That isn't a very large increase. Fair point, given how quickly it has grown, maybe something like 4k would give more headroom. > > > #define MAX_ALIASES 500 > > #define MAX_NODES 1024 > > > > @@ -1240,6 +1240,8 @@ static void read_slab_dir(void) > > p--; > > alias->ref = strdup(p); > > alias++; > > + if (alias - aliasinfo > MAX_ALIASES) > > + fatal("Too many aliases\n"); > > break; > > case DT_DIR: > > if (chdir(de->d_name)) > > @@ -1301,6 +1303,8 @@ static void read_slab_dir(void) > > if (slab->name[0] == ':') > > alias_targets++; > > slab++; > > + if (slab - slabinfo > MAX_SLABS) > > + fatal("Too many slabs\n"); > > break; > > This could be improved - if the number of slabs is exactly equal to > MAX_SLABS we'll unnecessarily report an error. Wouldn't this > alteration be better? Actually the issue is rather that we'll accept MAX_SLABS + 1 when we shouldn't, so yes the checks should be >=, or could also be just ==, sorry. > > --- a/tools/mm/slabinfo.c~tools-mm-fix-slabinfo-crash-when-max_slabs-is-exceeded-fix > +++ a/tools/mm/slabinfo.c > @@ -1228,6 +1228,8 @@ static void read_slab_dir(void) > continue; > switch (de->d_type) { > case DT_LNK: > + if (alias - aliasinfo >= MAX_ALIASES) > + fatal("Too many aliases\n"); > alias->name = strdup(de->d_name); > count = readlink(de->d_name, buffer, sizeof(buffer)-1); > > @@ -1240,10 +1242,10 @@ static void read_slab_dir(void) > p--; > alias->ref = strdup(p); > alias++; > - if (alias - aliasinfo > MAX_ALIASES) > - fatal("Too many aliases\n"); > break; > case DT_DIR: > + if (slab - slabinfo >= MAX_SLABS) > + fatal("Too many slabs\n"); > if (chdir(de->d_name)) > fatal("Unable to access slab %s\n", slab->name); > slab->name = strdup(de->d_name); > @@ -1305,8 +1307,6 @@ static void read_slab_dir(void) > if (slab->name[0] == ':') > alias_targets++; > slab++; > - if (slab - slabinfo > MAX_SLABS) > - fatal("Too many slabs\n"); > break; > default : > fatal("Unknown file type %lx\n", de->d_type); > _ >
diff --git a/tools/mm/slabinfo.c b/tools/mm/slabinfo.c index cfaeaea71042..2e0a1890dbbe 100644 --- a/tools/mm/slabinfo.c +++ b/tools/mm/slabinfo.c @@ -21,7 +21,7 @@ #include <regex.h> #include <errno.h> -#define MAX_SLABS 500 +#define MAX_SLABS 1000 #define MAX_ALIASES 500 #define MAX_NODES 1024 @@ -1240,6 +1240,8 @@ static void read_slab_dir(void) p--; alias->ref = strdup(p); alias++; + if (alias - aliasinfo > MAX_ALIASES) + fatal("Too many aliases\n"); break; case DT_DIR: if (chdir(de->d_name)) @@ -1301,6 +1303,8 @@ static void read_slab_dir(void) if (slab->name[0] == ':') alias_targets++; slab++; + if (slab - slabinfo > MAX_SLABS) + fatal("Too many slabs\n"); break; default : fatal("Unknown file type %lx\n", de->d_type); @@ -1310,10 +1314,6 @@ static void read_slab_dir(void) slabs = slab - slabinfo; actual_slabs = slabs; aliases = alias - aliasinfo; - if (slabs > MAX_SLABS) - fatal("Too many slabs\n"); - if (aliases > MAX_ALIASES) - fatal("Too many aliases\n"); } static void output_slabs(void)