| Message ID | 1353872669.30446.863.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
于 2012年11月26日 03:44, Eric Dumazet 写道: > From: Eric Dumazet <edumazet@google.com> > > Name of pimreg devices are built from following format : > > char name[IFNAMSIZ]; // IFNAMSIZ == 16 > > sprintf(name, "pimreg%u", mrt->id); > > We must therefore limit mrt->id to 9 decimal digits > or risk a buffer overflow and a crash. > > Restrict table identifiers in [0 ... 999999999] interval. > if we have to stick to "pimreg%u" (or will hurt the functional features) suggest to let user mode know this limitation. define a macro in public header (user mode can know it) and give comments. use macro instead of number. remove the comments which is inside internal function. thanks. gchen. > Reported-by: Chen Gang <gang.chen@asianux.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/ipv4/ipmr.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > index 6168c4d..3eab2b2 100644 > --- a/net/ipv4/ipmr.c > +++ b/net/ipv4/ipmr.c > @@ -1318,6 +1318,10 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi > if (get_user(v, (u32 __user *)optval)) > return -EFAULT; > > + /* "pimreg%u" should not exceed 16 bytes (IFNAMSIZ) */ > + if (v != RT_TABLE_DEFAULT && v >= 1000000000) > + return -EINVAL; > + > rtnl_lock(); > ret = 0; > if (sk == rtnl_dereference(mrt->mroute_sk)) { > > > >
于 2012年11月26日 09:22, Chen Gang 写道: > 于 2012年11月26日 03:44, Eric Dumazet 写道: >> From: Eric Dumazet <edumazet@google.com> >> >> Name of pimreg devices are built from following format : >> >> char name[IFNAMSIZ]; // IFNAMSIZ == 16 >> >> sprintf(name, "pimreg%u", mrt->id); >> >> We must therefore limit mrt->id to 9 decimal digits >> or risk a buffer overflow and a crash. >> >> Restrict table identifiers in [0 ... 999999999] interval. >> if "pimre%u" (or another format), will not hurt the functional features, I suggest to use it since, we need try our best to not touch the OS API. ("pimreg%u" seems an internal format, not OS API Level) > > if we have to stick to "pimreg%u" (or will hurt the functional features) > suggest to let user mode know this limitation. > define a macro in public header (user mode can know it) and give comments. > use macro instead of number. > remove the comments which is inside internal function. > > thanks. > > gchen. > > >> Reported-by: Chen Gang <gang.chen@asianux.com> >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> --- >> net/ipv4/ipmr.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c >> index 6168c4d..3eab2b2 100644 >> --- a/net/ipv4/ipmr.c >> +++ b/net/ipv4/ipmr.c >> @@ -1318,6 +1318,10 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi >> if (get_user(v, (u32 __user *)optval)) >> return -EFAULT; >> >> + /* "pimreg%u" should not exceed 16 bytes (IFNAMSIZ) */ >> + if (v != RT_TABLE_DEFAULT && v >= 1000000000) >> + return -EINVAL; >> + >> rtnl_lock(); >> ret = 0; >> if (sk == rtnl_dereference(mrt->mroute_sk)) { >> >> >> >> > >
On Mon, 2012-11-26 at 09:34 +0800, Chen Gang wrote: > if "pimre%u" (or another format), will not hurt the functional features, I suggest to use it > since, we need try our best to not touch the OS API. > ("pimreg%u" seems an internal format, not OS API Level) Have you taken a look at user code base before suggesting such a change ? My patch is the safest change: Just make sure machine doesnt crash if user ask stupid things. No possible regression. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年11月26日 10:19, Eric Dumazet 写道: > On Mon, 2012-11-26 at 09:34 +0800, Chen Gang wrote: > >> if "pimre%u" (or another format), will not hurt the functional features, I suggest to use it >> since, we need try our best to not touch the OS API. >> ("pimreg%u" seems an internal format, not OS API Level) > > Have you taken a look at user code base before suggesting such a > change ? maybe a hacker can do ? (I just guess). for my experience: It is necessary to think of more coding ways, when we have to give comments inside a function. > > My patch is the safest change: Just make sure machine doesnt crash if > user ask stupid things. > > No possible regression. > > > > >
On Mon, 2012-11-26 at 10:30 +0800, Chen Gang wrote: > maybe a hacker can do ? (I just guess). > > for my experience: > It is necessary to think of more coding ways, when we have to give comments inside a function. I have absolutely no idea of what you are trying to say. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年11月26日 10:55, Eric Dumazet 写道: > On Mon, 2012-11-26 at 10:30 +0800, Chen Gang wrote: > >> maybe a hacker can do ? (I just guess). >> >> for my experience: >> It is necessary to think of more coding ways, when we have to give comments inside a function. > > I have absolutely no idea of what you are trying to say. > excuse me, my English is not quite well. I will try to say it as clear as I can. what I want to say are: for os api, we need describe them as full as we can (including limitations, although it seems minor). I do not suggest to give comments inside a fuction (especially coding with hard code number). they are just my suggestion, not mean, it should be regression. at least, I think this patch is valuable.
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sun, 25 Nov 2012 11:44:29 -0800 > From: Eric Dumazet <edumazet@google.com> > > Name of pimreg devices are built from following format : > > char name[IFNAMSIZ]; // IFNAMSIZ == 16 > > sprintf(name, "pimreg%u", mrt->id); > > We must therefore limit mrt->id to 9 decimal digits > or risk a buffer overflow and a crash. > > Restrict table identifiers in [0 ... 999999999] interval. > > Reported-by: Chen Gang <gang.chen@asianux.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied, thanks Eric. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c index 6168c4d..3eab2b2 100644 --- a/net/ipv4/ipmr.c +++ b/net/ipv4/ipmr.c @@ -1318,6 +1318,10 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi if (get_user(v, (u32 __user *)optval)) return -EFAULT; + /* "pimreg%u" should not exceed 16 bytes (IFNAMSIZ) */ + if (v != RT_TABLE_DEFAULT && v >= 1000000000) + return -EINVAL; + rtnl_lock(); ret = 0; if (sk == rtnl_dereference(mrt->mroute_sk)) {