unused code in net/netfilter/ipset/ip_set_bitmap_ipmac.c

unused code in net/netfilter/ipset/ip_set_bitmap_ipmac.c

[Julia Lawall]

The file net/netfilter/ipset/ip_set_bitmap_ipmac.c seems to contain a lot
of static functions that are not used in the file:

bitmap_ipmac_add_timeout
bitmap_ipmac_do_add
bitmap_ipmac_do_del
bitmap_ipmac_do_head
bitmap_ipmac_do_list
bitmap_ipmac_do_test
bitmap_ipmac_gc_test
bitmap_ipmac_is_filled
bitmap_ipmac_kadt
bitmap_ipmac_same_set
bitmap_ipmac_uadt

Have I overooked something?

I was looking at this code, with Daniel Borkmann, because there seems to
be a bug in the function bitmap_ipmac_uadt:

	if (tb[IPSET_ATTR_ETHER]) {
                memcpy(e.ether, nla_data(tb[IPSET_ATTR_ETHER]), ETH_ALEN);
                e.add_mac = 1;
        }

Later in the same file, there is:

static struct ip_set_type bitmap_ipmac_type = {
	...
        .adt_policy     = {
		...
		[IPSET_ATTR_ETHER]      = { .type = NLA_BINARY,
                                            .len  = ETH_ALEN },
		...},
	...
};

The type NLA_BINARY indicates that the length is a maximum possible
length, and thus a check of the actual length is needed before the memcpy.

The file net/netfilter/ipset/ip_set_hash_mac.c seems to have a similar
problem.  The following static functions are not used:

hash_mac4_data_equal
hash_mac4_data_list
hash_mac4_data_next
hash_mac4_kadt
hash_mac4_uadt

And the following code:

ether_addr_copy(e.ether, nla_data(tb[IPSET_ATTR_ETHER]));

in hash_mac4_uadt does not seem to have a check on the length, and the
field is defined in the same way as above.

julia

Re: unused code in net/netfilter/ipset/ip_set_bitmap_ipmac.c

[Jozsef Kadlecsik]

Hi,

On Mon, 29 Feb 2016, Julia Lawall wrote:

> The file net/netfilter/ipset/ip_set_bitmap_ipmac.c seems to contain a lot
> of static functions that are not used in the file:
> 
> bitmap_ipmac_add_timeout
> bitmap_ipmac_do_add
> bitmap_ipmac_do_del
> bitmap_ipmac_do_head
> bitmap_ipmac_do_list
> bitmap_ipmac_do_test
> bitmap_ipmac_gc_test
> bitmap_ipmac_is_filled
> bitmap_ipmac_kadt
> bitmap_ipmac_same_set
> bitmap_ipmac_uadt
> 
> Have I overooked something?

Yes: the file includes ip_set_bitmap_gen.h in which all those functions 
are used.
 
> I was looking at this code, with Daniel Borkmann, because there seems to
> be a bug in the function bitmap_ipmac_uadt:
> 
> 	if (tb[IPSET_ATTR_ETHER]) {
>                 memcpy(e.ether, nla_data(tb[IPSET_ATTR_ETHER]), ETH_ALEN);
>                 e.add_mac = 1;
>         }
> 
> Later in the same file, there is:
> 
> static struct ip_set_type bitmap_ipmac_type = {
> 	...
>         .adt_policy     = {
> 		...
> 		[IPSET_ATTR_ETHER]      = { .type = NLA_BINARY,
>                                             .len  = ETH_ALEN },
> 		...},
> 	...
> };
> 
> The type NLA_BINARY indicates that the length is a maximum possible
> length, and thus a check of the actual length is needed before the memcpy.

You are right here (and the similar spotting in ip_set_hash_mac.c) - I'll 
prepare a patch and submit it.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: unused code in net/netfilter/ipset/ip_set_bitmap_ipmac.c

[Julia Lawall]

On Mon, 29 Feb 2016, Jozsef Kadlecsik wrote:

> Hi,
>
> On Mon, 29 Feb 2016, Julia Lawall wrote:
>
> > The file net/netfilter/ipset/ip_set_bitmap_ipmac.c seems to contain a lot
> > of static functions that are not used in the file:
> >
> > bitmap_ipmac_add_timeout
> > bitmap_ipmac_do_add
> > bitmap_ipmac_do_del
> > bitmap_ipmac_do_head
> > bitmap_ipmac_do_list
> > bitmap_ipmac_do_test
> > bitmap_ipmac_gc_test
> > bitmap_ipmac_is_filled
> > bitmap_ipmac_kadt
> > bitmap_ipmac_same_set
> > bitmap_ipmac_uadt
> >
> > Have I overooked something?
>
> Yes: the file includes ip_set_bitmap_gen.h in which all those functions
> are used.

OK, thanks.  I saw the incude, but I didn't sufficiently appreciate the
#defines at the beginning.  Thanks.

> > I was looking at this code, with Daniel Borkmann, because there seems to
> > be a bug in the function bitmap_ipmac_uadt:
> >
> > 	if (tb[IPSET_ATTR_ETHER]) {
> >                 memcpy(e.ether, nla_data(tb[IPSET_ATTR_ETHER]), ETH_ALEN);
> >                 e.add_mac = 1;
> >         }
> >
> > Later in the same file, there is:
> >
> > static struct ip_set_type bitmap_ipmac_type = {
> > 	...
> >         .adt_policy     = {
> > 		...
> > 		[IPSET_ATTR_ETHER]      = { .type = NLA_BINARY,
> >                                             .len  = ETH_ALEN },
> > 		...},
> > 	...
> > };
> >
> > The type NLA_BINARY indicates that the length is a maximum possible
> > length, and thus a check of the actual length is needed before the memcpy.
>
> You are right here (and the similar spotting in ip_set_hash_mac.c) - I'll
> prepare a patch and submit it.

Great.  Thanks.

julia

>
> Best regards,
> Jozsef
> -
> E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
> PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
> Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
>           H-1525 Budapest 114, POB. 49, Hungary
>