[rfc] netfilter: copy less data to the user

[rfc] netfilter: copy less data to the user

[Dan Carpenter]

Smatch complains that we copy too much data to the user in ebtables.
We copied EBT_FUNCTION_MAXNAMELEN (32) characters to the user here, but 
"m->u.match->name" has XT_EXTENSION_MAXNAMELEN (29) characters.

I'm not sure if this is a bug where someone got confused with m->u.name
which has 32 characters or if this is done for backwards compatability.

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 59ca00e..6bcb31d 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1323,7 +1323,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)m - base);
-	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1332,7 +1332,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)w - base);
-	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1356,7 +1356,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
 	if (ret != 0)
 		return ret;
-	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }

Re: [rfc] netfilter: copy less data to the user

[Patrick McHardy]

Am 14.07.2010 23:04, schrieb Dan Carpenter:
> Smatch complains that we copy too much data to the user in ebtables.
> We copied EBT_FUNCTION_MAXNAMELEN (32) characters to the user here, but 
> "m->u.match->name" has XT_EXTENSION_MAXNAMELEN (29) characters.
> 
> I'm not sure if this is a bug where someone got confused with m->u.name
> which has 32 characters or if this is done for backwards compatability.

Looking at ebtables.h, ebt_entry_match->name uses
EBT_FUNCTION_MAXNAMELEN, which is 32 bytes. Where did you get
XT_EXTENSION_MAXNAMELEN from?

> 
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 59ca00e..6bcb31d 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1323,7 +1323,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
>      const char *base, char __user *ubase)
>  {
>  	char __user *hlp = ubase + ((char *)m - base);
> -	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> @@ -1332,7 +1332,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
>      const char *base, char __user *ubase)
>  {
>  	char __user *hlp = ubase + ((char *)w - base);
> -	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> @@ -1356,7 +1356,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
>  	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
>  	if (ret != 0)
>  		return ret;
> -	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [rfc] netfilter: copy less data to the user

[Dan Carpenter]

On Thu, Jul 15, 2010 at 11:48:09AM +0200, Patrick McHardy wrote:
> Am 14.07.2010 23:04, schrieb Dan Carpenter:
> > Smatch complains that we copy too much data to the user in ebtables.
> > We copied EBT_FUNCTION_MAXNAMELEN (32) characters to the user here, but 
> > "m->u.match->name" has XT_EXTENSION_MAXNAMELEN (29) characters.
> > 
> > I'm not sure if this is a bug where someone got confused with m->u.name
> > which has 32 characters or if this is done for backwards compatability.
> 
> Looking at ebtables.h, ebt_entry_match->name uses
> EBT_FUNCTION_MAXNAMELEN, which is 32 bytes. Where did you get
> XT_EXTENSION_MAXNAMELEN from?
> 

Exactly.  ebt_entry_match->u.name uses EBT_FUNCTION_MAXNAMELEN but this is
from ebt_entry_match->u.match->name which is type struct xt_match.

But it looks like we're exporting struct ebt_match which also uses
EBT_FUNCTION_MAXNAMELEN.  So maybe the fix is to copy ->u.name instead
of ->u.match->name.

regards,
dan carpenter

Re: [rfc] netfilter: copy less data to the user

[Patrick McHardy]

Am 15.07.2010 12:16, schrieb Dan Carpenter:
> On Thu, Jul 15, 2010 at 11:48:09AM +0200, Patrick McHardy wrote:
>> Am 14.07.2010 23:04, schrieb Dan Carpenter:
>>> Smatch complains that we copy too much data to the user in ebtables.
>>> We copied EBT_FUNCTION_MAXNAMELEN (32) characters to the user here, but 
>>> "m->u.match->name" has XT_EXTENSION_MAXNAMELEN (29) characters.
>>>
>>> I'm not sure if this is a bug where someone got confused with m->u.name
>>> which has 32 characters or if this is done for backwards compatability.
>>
>> Looking at ebtables.h, ebt_entry_match->name uses
>> EBT_FUNCTION_MAXNAMELEN, which is 32 bytes. Where did you get
>> XT_EXTENSION_MAXNAMELEN from?
>>
> 
> Exactly.  ebt_entry_match->u.name uses EBT_FUNCTION_MAXNAMELEN but this is
> from ebt_entry_match->u.match->name which is type struct xt_match.

Right, I see.

> But it looks like we're exporting struct ebt_match which also uses
> EBT_FUNCTION_MAXNAMELEN.  So maybe the fix is to copy ->u.name instead
> of ->u.match->name.

That name is not valid within the kernel, the union contains the
xt_match pointer. So your patch seems correct, but we probably
also need to adjust ebtables userspace.

Jan?

Re: [rfc] netfilter: copy less data to the user

[Jan Engelhardt]

On Thursday 2010-07-15 17:29, Patrick McHardy wrote:
>Am 15.07.2010 12:16, schrieb Dan Carpenter:
>> On Thu, Jul 15, 2010 at 11:48:09AM +0200, Patrick McHardy wrote:
>>> Am 14.07.2010 23:04, schrieb Dan Carpenter:
>>>> Smatch complains that we copy too much data to the user in ebtables.
>>>> We copied EBT_FUNCTION_MAXNAMELEN (32) characters to the user here, but 
>>>> "m->u.match->name" has XT_EXTENSION_MAXNAMELEN (29) characters.
>>>>
>>>> I'm not sure if this is a bug where someone got confused with m->u.name
>>>> which has 32 characters or if this is done for backwards compatability.
>>>
>>> Looking at ebtables.h, ebt_entry_match->name uses
>>> EBT_FUNCTION_MAXNAMELEN, which is 32 bytes. Where did you get
>>> XT_EXTENSION_MAXNAMELEN from?
>>>
>> 
>> Exactly.  ebt_entry_match->u.name uses EBT_FUNCTION_MAXNAMELEN but this is
>> from ebt_entry_match->u.match->name which is type struct xt_match.
>
>Right, I see.
>
>> But it looks like we're exporting struct ebt_match which also uses
>> EBT_FUNCTION_MAXNAMELEN.  So maybe the fix is to copy ->u.name instead
>> of ->u.match->name.
>
>That name is not valid within the kernel, the union contains the
>xt_match pointer. So your patch seems correct, but we probably
>also need to adjust ebtables userspace.
>
>Jan?

Hm this mail is not marked in my MUA as being answered, though I could 
swear I replied. Well again:

I'd say copy 29 chars from the name, and fill the rest with \0s.