Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Christoph Lameter]

The following code casts pointers to char to long in order to do a fast
comparison. This causes alignment errors on IA64 and likely also on
other platforms:

        /* Look for ifname matches; this should unroll nicely. */
        for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
                ret |= (((const unsigned long *)indev)[i]
                        ^ ((const unsigned long *)arpinfo->iniface)[i])
                        & ((const unsigned long *)arpinfo->iniface_mask)[i];
        }

iniface is a pointer to char.

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Alex Williamson]

   A patch for this was sent to the netfilter-devel list a while back,
see this link:

http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2

I don't think the patch ever made it anywhere so far.

	Alex

On Wed, 2004-06-09 at 12:09, Christoph Lameter wrote:
> The following code casts pointers to char to long in order to do a fast
> comparison. This causes alignment errors on IA64 and likely also on
> other platforms:
> 
>         /* Look for ifname matches; this should unroll nicely. */
>         for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
>                 ret |= (((const unsigned long *)indev)[i]
>                         ^ ((const unsigned long *)arpinfo->iniface)[i])
>                         & ((const unsigned long *)arpinfo->iniface_mask)[i];
>         }
> 
> iniface is a pointer to char.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[David S. Miller]

On Wed, 09 Jun 2004 12:27:56 -0600
Alex Williamson <alex.williamson@hp.com> wrote:

> http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2

How can you legitimately change this structure?  It's an exported
userland interface, if you change it all the applications will
stop working.

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Alex Williamson]

On Wed, 2004-06-09 at 14:00, David S. Miller wrote:
> On Wed, 09 Jun 2004 12:27:56 -0600
> Alex Williamson <alex.williamson@hp.com> wrote:
> 
> > http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2
> 
> How can you legitimately change this structure?  It's an exported
> userland interface, if you change it all the applications will
> stop working.
> 

   Which is probably why the patch never went anywhere.  There's
certainly an alignment issue in the usage of the struct arpt_arp in the
code snippet Christoph pointed out.  Sounds like it'd be better to fix
the usage than the structure alignment.

	Alex

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[David S. Miller]

On Wed, 09 Jun 2004 14:29:36 -0600
Alex Williamson <alex.williamson@hp.com> wrote:

>    Which is probably why the patch never went anywhere.  There's
> certainly an alignment issue in the usage of the struct arpt_arp in the
> code snippet Christoph pointed out.  Sounds like it'd be better to fix
> the usage than the structure alignment.

Right.  I distinctly remember a similar fix being needed to
ip_tables.c many months ago, a search though the change history
for that file might prove profitable :-)

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On Wed, Jun 09, 2004 at 01:29:37PM -0700, David S. Miller wrote:
> On Wed, 09 Jun 2004 14:29:36 -0600
> Alex Williamson <alex.williamson@hp.com> wrote:
> 
> >    Which is probably why the patch never went anywhere.  There's
> > certainly an alignment issue in the usage of the struct arpt_arp in the
> > code snippet Christoph pointed out.  Sounds like it'd be better to fix
> > the usage than the structure alignment.
> 
> Right.  I distinctly remember a similar fix being needed to
> ip_tables.c many months ago, a search though the change history
> for that file might prove profitable :-)

Or alternatively look into the netfilter bugzilla at:

https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=84

If somebody wants to prepare a trivial merge of that fix with arptables
- it should be extermely straight forward ;)

-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Alex Williamson]

On Wed, 2004-06-09 at 15:33, Harald Welte wrote:
> On Wed, Jun 09, 2004 at 01:29:37PM -0700, David S. Miller wrote:
> > 
> > Right.  I distinctly remember a similar fix being needed to
> > ip_tables.c many months ago, a search though the change history
> > for that file might prove profitable :-)
> 
> Or alternatively look into the netfilter bugzilla at:
> 
> https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=84
> 
> If somebody wants to prepare a trivial merge of that fix with arptables
> - it should be extermely straight forward ;)

   That change is probably appropriate, but IIRC, that's not the
alignment problem I saw, and that one certainly wouldn't have been fixed
by a change to the arpt_arp structure.  Looking at the code snippet
again:

/* Look for ifname matches; this should unroll nicely. */
for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
        ret |= (((const unsigned long *)indev)[i]
                ^ ((const unsigned long *)arpinfo->iniface)[i])
                & ((const unsigned long *)arpinfo->iniface_mask)[i];
}

The alignment problem I remember was with iniface and iniface_mask.  If
we can't change the structure alignment, the easiest fix is to change
the stride length to something appropriate for the arch or maybe just a
least common demoninator.  Maybe someone is smart enough to get the
preprocessor to figure this out automatically...  dunno if that's
possible.

   While we're on this little piece of code, there's another bug here. 
ret is defined as an int in arp_packet_match() so we're losing the upper
half of the result anyway.  ip_packet_match() appears to already have
this correct.  At a minimum, I think we need the trivial patch below. 
Thanks,

	Alex

===== net/ipv4/netfilter/arp_tables.c 1.13 vs edited =====
--- 1.13/net/ipv4/netfilter/arp_tables.c	Sun Jun  6 21:15:04 2004
+++ edited/net/ipv4/netfilter/arp_tables.c	Wed Jun  9 15:38:16 2004
@@ -106,7 +106,8 @@
 	char *arpptr = (char *)(arphdr + 1);
 	char *src_devaddr, *tgt_devaddr;
 	u32 *src_ipaddr, *tgt_ipaddr;
-	int i, ret;
+	int i;
+	unsigned long ret;
 
 #define FWINV(bool,invflg) ((bool) ^ !!(arpinfo->invflags & invflg))
 

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Chris Wedgwood]

On Wed, Jun 09, 2004 at 01:00:01PM -0700, David S. Miller wrote:

> How can you legitimately change this structure?  It's an exported
> userland interface, if you change it all the applications will stop
> working.

Why not split the structure for user-space and kernel-space version
and cp/frob at/near the syscall boundary?



  --cw

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Wed, Jun 09, 2004 at 06:45:19PM -0700, Chris Wedgwood wrote:
> On Wed, Jun 09, 2004 at 01:00:01PM -0700, David S. Miller wrote:
> 
> > How can you legitimately change this structure?  It's an exported
> > userland interface, if you change it all the applications will stop
> > working.
> 
> Why not split the structure for user-space and kernel-space version
> and cp/frob at/near the syscall boundary?

because it would look like an ugly hack in the setsockopt call, plus
adding another costly/time consuming parse of the table BLOB.  

Also note that the kernel currently has no code that supports the
generation/modification of rulesets. All it can do is iterate over them.

>   --cw

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[David S. Miller]

On Wed, 9 Jun 2004 11:09:42 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

>         /* Look for ifname matches; this should unroll nicely. */
>         for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
>                 ret |= (((const unsigned long *)indev)[i]
>                         ^ ((const unsigned long *)arpinfo->iniface)[i])
>                         & ((const unsigned long *)arpinfo->iniface_mask)[i];
>         }

This is far from a critical code path, so this is how I'm
going to fix this.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/06/10 22:05:19-07:00 davem@nuts.davemloft.net 
#   [IPV4]: Fix unaligned accesses in arp_tables.c
# 
# net/ipv4/netfilter/arp_tables.c
#   2004/06/10 22:05:03-07:00 davem@nuts.davemloft.net +3 -4
#   [IPV4]: Fix unaligned accesses in arp_tables.c
# 
diff -Nru a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
--- a/net/ipv4/netfilter/arp_tables.c	2004-06-10 22:05:40 -07:00
+++ b/net/ipv4/netfilter/arp_tables.c	2004-06-10 22:05:40 -07:00
@@ -179,11 +179,10 @@
 		return 0;
 	}
 
-	/* Look for ifname matches; this should unroll nicely. */
+	/* Look for ifname matches.  */
 	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)indev)[i]
-			^ ((const unsigned long *)arpinfo->iniface)[i])
-			& ((const unsigned long *)arpinfo->iniface_mask)[i];
+		ret |= (indev[i] ^ arpinfo->iniface[i])
+			& arpinfo->iniface_mask[i];
 	}
 
 	if (FWINV(ret != 0, ARPT_INV_VIA_IN)) {

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

On Jun 10, 2004  22:04 -0700, David S. Miller wrote:
> This is far from a critical code path, so this is how I'm
> going to fix this.
> 
> # This is a BitKeeper generated diff -Nru style patch.
> #
> # ChangeSet
> #   2004/06/10 22:05:19-07:00 davem@nuts.davemloft.net 
> #   [IPV4]: Fix unaligned accesses in arp_tables.c
> # 
> # net/ipv4/netfilter/arp_tables.c
> #   2004/06/10 22:05:03-07:00 davem@nuts.davemloft.net +3 -4
> #   [IPV4]: Fix unaligned accesses in arp_tables.c
> # 
> diff -Nru a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> --- a/net/ipv4/netfilter/arp_tables.c	2004-06-10 22:05:40 -07:00
> +++ b/net/ipv4/netfilter/arp_tables.c	2004-06-10 22:05:40 -07:00
> @@ -179,11 +179,10 @@
>  		return 0;
>  	}
>  
> -	/* Look for ifname matches; this should unroll nicely. */
> +	/* Look for ifname matches.  */
>  	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> -		ret |= (((const unsigned long *)indev)[i]
> -			^ ((const unsigned long *)arpinfo->iniface)[i])
> -			& ((const unsigned long *)arpinfo->iniface_mask)[i];
> +		ret |= (indev[i] ^ arpinfo->iniface[i])
> +			& arpinfo->iniface_mask[i];
>  	}

- 	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
+ 	for (i = 0, ret = 0; i < IFNAMSIZ; i++) {

Shouldn't your change include the above?

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/                 http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Unaligned accesses in net/ipv4/netfilter/arp_tables.c:184

[David S. Miller]

On Thu, 10 Jun 2004 23:41:11 -0600
Andreas Dilger <adilger@clusterfs.com> wrote:

> - 	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> + 	for (i = 0, ret = 0; i < IFNAMSIZ; i++) {
> 
> Shouldn't your change include the above?

Yes, I'm retarted, thanks for catching that.