* [Suggestion] net/ipv4: sprintf, use "pimreg%.9u" instead of "pimreg%u".
@ 2012-11-21 9:20 Chen Gang
2012-11-25 19:44 ` [PATCH] net: ipmr: limit MRT_TABLE identifiers Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Chen Gang @ 2012-11-21 9:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-21 9:20 [Suggestion] net/ipv4: sprintf, use "pimreg%.9u" instead of "pimreg%u" Chen Gang
@ 2012-11-25 19:44 ` Eric Dumazet
2012-11-26 1:22 ` Chen Gang
2012-11-26 22:37 ` David Miller
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2012-11-25 19:44 UTC (permalink / raw)
To: Chen Gang; +Cc: David Miller, netdev
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>
---
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)) {
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-25 19:44 ` [PATCH] net: ipmr: limit MRT_TABLE identifiers Eric Dumazet
@ 2012-11-26 1:22 ` Chen Gang
2012-11-26 1:34 ` Chen Gang
2012-11-26 22:37 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Chen Gang @ 2012-11-26 1:22 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
于 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)) {
>
>
>
>
--
Chen Gang
Asianux Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-26 1:22 ` Chen Gang
@ 2012-11-26 1:34 ` Chen Gang
2012-11-26 2:19 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Chen Gang @ 2012-11-26 1:34 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
于 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)) {
>>
>>
>>
>>
>
>
--
Chen Gang
Asianux Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-26 1:34 ` Chen Gang
@ 2012-11-26 2:19 ` Eric Dumazet
2012-11-26 2:30 ` Chen Gang
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-11-26 2:19 UTC (permalink / raw)
To: Chen Gang; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-26 2:19 ` Eric Dumazet
@ 2012-11-26 2:30 ` Chen Gang
2012-11-26 2:55 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Chen Gang @ 2012-11-26 2:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
于 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.
>
>
>
>
>
--
Chen Gang
Asianux Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-26 2:30 ` Chen Gang
@ 2012-11-26 2:55 ` Eric Dumazet
2012-11-26 3:13 ` Chen Gang
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-11-26 2:55 UTC (permalink / raw)
To: Chen Gang; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-26 2:55 ` Eric Dumazet
@ 2012-11-26 3:13 ` Chen Gang
0 siblings, 0 replies; 9+ messages in thread
From: Chen Gang @ 2012-11-26 3:13 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
于 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.
--
Chen Gang
Asianux Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ipmr: limit MRT_TABLE identifiers
2012-11-25 19:44 ` [PATCH] net: ipmr: limit MRT_TABLE identifiers Eric Dumazet
2012-11-26 1:22 ` Chen Gang
@ 2012-11-26 22:37 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2012-11-26 22:37 UTC (permalink / raw)
To: eric.dumazet; +Cc: gang.chen, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-26 22:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-21 9:20 [Suggestion] net/ipv4: sprintf, use "pimreg%.9u" instead of "pimreg%u" Chen Gang
2012-11-25 19:44 ` [PATCH] net: ipmr: limit MRT_TABLE identifiers Eric Dumazet
2012-11-26 1:22 ` Chen Gang
2012-11-26 1:34 ` Chen Gang
2012-11-26 2:19 ` Eric Dumazet
2012-11-26 2:30 ` Chen Gang
2012-11-26 2:55 ` Eric Dumazet
2012-11-26 3:13 ` Chen Gang
2012-11-26 22:37 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox