[Suggestion] net/ipv4: sprintf, use "pimreg%.9u" instead of "pimreg%u".

Hello David Miller:

in net/ipv4/ipmr.c:
  the mrt->id is u32 (at line 78),
  the length of name is 16 (IFNAMESIZ, line 489)
  mrt->id can be larger than 999999999, such as 4294967294 (0xffffffff - 1)
  so the len of "pimreg%u" can be 17 (pimreg4294967294'\0', line 494)

another information:
  the mrt->id is assigned in ipmr_new_table, without checking its value region (line 309)
  one calling work flow is ip_mroute_setsockopt (line 1202) -> ipmr_new_table (line 1326)
  RT_TABLE_* are as enum for mrt->id using, (include/uapi/linux/rtnetlink.h:255)

  73 struct mr_table {
  74         struct list_head        list;
  75 #ifdef CONFIG_NET_NS
  76         struct net              *net;
  77 #endif
  78         u32                     id;
  79         struct sock __rcu       *mroute_sk;
  80         struct timer_list       ipmr_expire_timer;
  81         struct list_head        mfc_unres_queue;
  82         struct list_head        mfc_cache_array[MFC_LINES];
  83         struct vif_device       vif_table[MAXVIFS];
  84         int                     maxvif;
  85         atomic_t                cache_resolve_queue_len;
  86         int                     mroute_do_assert;
  87         int                     mroute_do_pim;
  88 #if defined(CONFIG_IP_PIMSM_V1) || defined(CONFIG_IP_PIMSM_V2)
  89         int                     mroute_reg_vif_num;
  90 #endif
  91 };
 ...

 309 static struct mr_table *ipmr_new_table(struct net *net, u32 id)
 310 {
 311         struct mr_table *mrt;
 312         unsigned int i;
 313 
 314         mrt = ipmr_get_table(net, id);
 315         if (mrt != NULL)
 316                 return mrt;
 317 
 318         mrt = kzalloc(sizeof(*mrt), GFP_KERNEL);
 319         if (mrt == NULL)
 320                 return NULL;
 321         write_pnet(&mrt->net, net);
 322         mrt->id = id;
 323 
 324         /* Forwarding cache */
 325         for (i = 0; i < MFC_LINES; i++)
 326                 INIT_LIST_HEAD(&mrt->mfc_cache_array[i]);
 327 
 328         INIT_LIST_HEAD(&mrt->mfc_unres_queue);
 329 
 330         setup_timer(&mrt->ipmr_expire_timer, ipmr_expire_process,
 331                     (unsigned long)mrt);
 332 
 333 #ifdef CONFIG_IP_PIMSM
 334         mrt->mroute_reg_vif_num = -1;
 335 #endif
 336 #ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
 337         list_add_tail_rcu(&mrt->list, &net->ipv4.mr_tables);
 338 #endif
 339         return mrt;
 340 }
 341 
 ...

 485 static struct net_device *ipmr_reg_vif(struct net *net, struct mr_table *mrt)
 486 {
 487         struct net_device *dev;
 488         struct in_device *in_dev;
 489         char name[IFNAMSIZ];
 490 
 491         if (mrt->id == RT_TABLE_DEFAULT)
 492                 sprintf(name, "pimreg");
 493         else
 494                 sprintf(name, "pimreg%u", mrt->id);
 495 
 496         dev = alloc_netdev(0, name, reg_vif_setup);
 497 
 498         if (dev == NULL)
 499                 return NULL;
 500 
 ...

1202 int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsigned int optlen)
1203 {
1204         int ret;
1205         struct vifctl vif;
1206         struct mfcctl mfc;
1207         struct net *net = sock_net(sk);
1208         struct mr_table *mrt;
1209 
1210         mrt = ipmr_get_table(net, raw_sk(sk)->ipmr_table ? : RT_TABLE_DEFAULT);
1211         if (mrt == NULL)
1212                 return -ENOENT;
1213 
1214         if (optname != MRT_INIT) {
1215                 if (sk != rcu_access_pointer(mrt->mroute_sk) &&
1216                     !capable(CAP_NET_ADMIN))
1217                         return -EACCES;
1218         }
1219 
1220         switch (optname) {

 ...

1311 #ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
1312         case MRT_TABLE:
1313         {
1314                 u32 v;
1315 
1316                 if (optlen != sizeof(u32))
1317                         return -EINVAL;
1318                 if (get_user(v, (u32 __user *)optval))
1319                         return -EFAULT;
1320 
1321                 rtnl_lock();
1322                 ret = 0;
1323                 if (sk == rtnl_dereference(mrt->mroute_sk)) {
1324                         ret = -EBUSY;
1325                 } else {
1326                         if (!ipmr_new_table(net, v))
1327                                 ret = -ENOMEM;
1328                         raw_sk(sk)->ipmr_table = v;
1329                 }
1330                 rtnl_unlock();
1331                 return ret;
1332         }
1333 #endif


in include/uapi/linux/rtnetlink.h

255 enum rt_class_t {
256         RT_TABLE_UNSPEC=0,
257 /* User defined values */
258         RT_TABLE_COMPAT=252,
259         RT_TABLE_DEFAULT=253,
260         RT_TABLE_MAIN=254,
261         RT_TABLE_LOCAL=255,
262         RT_TABLE_MAX=0xFFFFFFFF
263 };
264 

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 <redacted>
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日 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.

-- 
Chen Gang

Asianux Corporation

于 2012年11月26日 09:22, Chen Gang 写道:

于 2012年11月26日 03:44, Eric Dumazet 写道:

quoted

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.

quoted

Reported-by: Chen Gang <redacted>
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)) {

-- 
Chen Gang

Asianux Corporation

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.

于 2012年11月26日 10:19, Eric Dumazet 写道:

On Mon, 2012-11-26 at 09:34 +0800, Chen Gang wrote:

quoted

    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.

-- 
Chen Gang

Asianux Corporation

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.

于 2012年11月26日 10:55, Eric Dumazet 写道:

On Mon, 2012-11-26 at 10:30 +0800, Chen Gang wrote:

quoted

  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.


-- 
Chen Gang

Asianux Corporation

From: Eric Dumazet <redacted>
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 <redacted>
Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.