[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;
}
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Patrick McHardy]

Post by 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?

Post by Dan Carpenter
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
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

Post by Patrick McHardy

Post by 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


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Patrick McHardy]

Post by Dan Carpenter

Post by Patrick McHardy

Post by 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.

Post by Dan Carpenter
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?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jan Engelhardt]

Post by Patrick McHardy

Post by Dan Carpenter

Post by Patrick McHardy

Post by 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.

Post by Dan Carpenter
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html