[PATCH] dm mpath: add check for count of groups to avoid wild pointer access

[PATCH] dm mpath: add check for count of groups to avoid wild pointer access

[tang.junhui]

From: "tang.junhui" <tang.junhui@zte.com.cn>

pg is not assigned to a group address when count of multipath groups
is zero in bypass_pg_num(), then it is used in bypass_pg(), which may
cause wild pointer access.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
 drivers/md/dm-mpath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index d376dc8..8c1359c 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1084,7 +1084,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr)
 	char dummy;
 
 	if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum ||
-	    (pgnum > m->nr_priority_groups)) {
+	    !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) {
 		DMWARN("invalid PG number supplied to switch_pg_num");
 		return -EINVAL;
 	}
-- 
2.8.1.windows.1

Re: dm mpath: add check for count of groups to avoid wild pointer access

[Mike Snitzer]

On Thu, Nov 03 2016 at  6:49am -0400,
tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:

> From: "tang.junhui" <tang.junhui@zte.com.cn>
> 
> pg is not assigned to a group address when count of multipath groups
> is zero in bypass_pg_num(), then it is used in bypass_pg(), which may
> cause wild pointer access.
> 
> Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
> ---
>  drivers/md/dm-mpath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index d376dc8..8c1359c 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -1084,7 +1084,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr)
>  	char dummy;
>  
>  	if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum ||
> -	    (pgnum > m->nr_priority_groups)) {
> +	    !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) {
>  		DMWARN("invalid PG number supplied to switch_pg_num");
>  		return -EINVAL;
>  	}
> -- 
> 2.8.1.windows.1
> 
> 

You mention bypass_pg_num() going on to hit a NULL/"wild" pointer.  Not
immediately seeing the relation between switch_pg_num() and
bypass_pg_num().  But shouldn't bypass_pg_num() have improved bounds
checking (and/or NULL pointer checks) too?

Maybe your patch was applied with an offset and it modified
switch_pg_num() when you really meant to modify bypass_pg_num()?

答复: Re: dm mpath: add check for count of groups to avoid wild pointer access

[tang.junhui]

[-- Attachment #1.1: Type: text/plain, Size: 2112 bytes --]

Hello Mike,

I'm sorry to send you the wrong patch,
I'll send a new patch to you later.

Thanks
Tang



发件人:         Mike Snitzer <snitzer@redhat.com>
收件人:         tang.junhui@zte.com.cn, 
抄送:   zhang.kai16@zte.com.cn, dm-devel@redhat.com, agk@redhat.com
日期:   2016/11/03 23:33
主题:   Re: [dm-devel] dm mpath: add check for count of groups to avoid 
wild pointer access
发件人: dm-devel-bounces@redhat.com



On Thu, Nov 03 2016 at  6:49am -0400,
tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:

> From: "tang.junhui" <tang.junhui@zte.com.cn>
> 
> pg is not assigned to a group address when count of multipath groups
> is zero in bypass_pg_num(), then it is used in bypass_pg(), which may
> cause wild pointer access.
> 
> Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
> ---
>  drivers/md/dm-mpath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index d376dc8..8c1359c 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -1084,7 +1084,7 @@ static int switch_pg_num(struct multipath *m, 
const char *pgstr)
>                char dummy;
> 
>                if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 
1) || !pgnum ||
> -                  (pgnum > m->nr_priority_groups)) {
> +                  !m->nr_priority_groups || (pgnum > 
m->nr_priority_groups)) {
>                                DMWARN("invalid PG number supplied to 
switch_pg_num");
>                                return -EINVAL;
>                }
> -- 
> 2.8.1.windows.1
> 
> 

You mention bypass_pg_num() going on to hit a NULL/"wild" pointer.  Not
immediately seeing the relation between switch_pg_num() and
bypass_pg_num().  But shouldn't bypass_pg_num() have improved bounds
checking (and/or NULL pointer checks) too?

Maybe your patch was applied with an offset and it modified
switch_pg_num() when you really meant to modify bypass_pg_num()?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



[-- Attachment #1.2: Type: text/html, Size: 4060 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]