| Message ID | 20220327053742.2942-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | md: fix missing check on list iterator | expand |
On Sun, Mar 27 2022 at 1:37P -0400, Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote: > The bug is here: > bypass_pg(m, pg, bypassed); > > The list iterator 'pg' will point to a bogus position containing > HEAD if the list is empty or no element is found. This case must > be checked before any use of the iterator, otherwise it will lead > to a invalid memory access. > > To fix this bug, run bypass_pg(m, pg, bypassed); and return 0 > when found, otherwise return -EINVAL. > > Cc: stable@vger.kernel.org > Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > --- > drivers/md/dm-mpath.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index f4719b65e5e3..6ba8f1133564 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -1496,12 +1496,13 @@ static int bypass_pg_num(struct multipath *m, const char *pgstr, bool bypassed) > } > > list_for_each_entry(pg, &m->priority_groups, list) { > - if (!--pgnum) > - break; > + if (!--pgnum) { > + bypass_pg(m, pg, bypassed); > + return 0; > + } > } > > - bypass_pg(m, pg, bypassed); > - return 0; > + return -EINVAL; > } > > /* > -- > 2.17.1 > Did you acually hit a bug (invalid memory access)? I cannot see how given the checks prior to iterating m->priority_groups: if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum || !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) { DMWARN("invalid PG number supplied to bypass_pg"); return -EINVAL; } So I have _not_ taken your "fix". -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, 1 Apr 2022 10:47:18 -0400, Mike Snitzer wrote: > Did you acually hit a bug (invalid memory access)? > > I cannot see how given the checks prior to iterating m->priority_groups: > > if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum || > !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) { > DMWARN("invalid PG number supplied to bypass_pg"); > return -EINVAL; > } > > So I have _not_ taken your "fix". Yes, you are correct. It has been checked before, thus not a bug and no need to fix. And I have sent a PATCH v2 to use list iterator only inside the loop, please check it. Thank you very much. -- Xiaomeng Tong -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index f4719b65e5e3..6ba8f1133564 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -1496,12 +1496,13 @@ static int bypass_pg_num(struct multipath *m, const char *pgstr, bool bypassed) } list_for_each_entry(pg, &m->priority_groups, list) { - if (!--pgnum) - break; + if (!--pgnum) { + bypass_pg(m, pg, bypassed); + return 0; + } } - bypass_pg(m, pg, bypassed); - return 0; + return -EINVAL; } /*
The bug is here: bypass_pg(m, pg, bypassed); The list iterator 'pg' will point to a bogus position containing HEAD if the list is empty or no element is found. This case must be checked before any use of the iterator, otherwise it will lead to a invalid memory access. To fix this bug, run bypass_pg(m, pg, bypassed); and return 0 when found, otherwise return -EINVAL. Cc: stable@vger.kernel.org Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> --- drivers/md/dm-mpath.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)