[PATCH 1/1] net: netlink: Fix multicast group storage allocation for families with more than one groups

[PATCH 1/1] net: netlink: Fix multicast group storage allocation for families with more than one groups

[Matti Vaittinen]

Hello,

    I got an oops when developing a driver utilizing more than 32
    multicast groups. After a little print adding I saw that part of 
    the address of policy struct was overwritten. It seems to me that
    amount of longs needed to store the multicast addresses is
    calculated wrong for families with more than one multicast groups.
    Problem is easy to reproduce by: 
    
    1. generating a module which registers large amount of multicast 
    groups.
    2. Loading the module
    3. Sending the netlink message which requests multicast groups for
    this family.

    I believe step 3 may not be even needed if there is enough of the
    groups registered.

    Patch created against just cloned net repository below.

    Br.
        Matti Vaittinen



Multicast groups are stored in global buffer. Check for needed buffer size
incorrectly compares buffer size to first id for family. This means that
for families with more than one mcast id one may allocate too small buffer
and end up writing rest of the groups to some unallocated memory. Fix the 
buffer size check to compare allocated space to last mcast id for the 
family.

Tested on ARM using kernel 3.14

Signed-off-by: Matti Vaittinen <matti.vaittinen@nokia.com>
---
 net/netlink/genetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index bc0e504..a992083 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -185,7 +185,7 @@ static int genl_allocate_reserve_groups(int n_groups, int *first_id)
 			}
 		}
 
-		if (id >= mc_groups_longs * BITS_PER_LONG) {
+		if (id + n_groups >= mc_groups_longs * BITS_PER_LONG) {
 			unsigned long new_longs = mc_groups_longs +
 						  BITS_TO_LONGS(n_groups);
 			size_t nlen = new_longs * sizeof(unsigned long);
-- 
2.1.0

Re: [PATCH 1/1] net: netlink: Fix multicast group storage allocation for families with more than one groups

[David Miller]

From: Matti Vaittinen <matti.vaittinen@nokia.com>
Date: Mon, 11 Jan 2016 14:26:19 +0200

> Multicast groups are stored in global buffer. Check for needed buffer size
> incorrectly compares buffer size to first id for family. This means that
> for families with more than one mcast id one may allocate too small buffer
> and end up writing rest of the groups to some unallocated memory. Fix the 
> buffer size check to compare allocated space to last mcast id for the 
> family.
> 
> Tested on ARM using kernel 3.14
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@nokia.com>

Indeed, it looks like this function was never tested with any value
of n_groups other than one.

But I think your change has an off-by-one bug:

> -		if (id >= mc_groups_longs * BITS_PER_LONG) {
> +		if (id + n_groups >= mc_groups_longs * BITS_PER_LONG) {

I think this needs to be "id + n_groups > ".  Consider the existing,
working, case of "n_groups == 1".  Now you're adding '1' and therefore
the test needs to be adjusted from >= to >.

Thanks.

Re: [PATCH 1/1] net: netlink: Fix multicast group storage allocation for families with more than one groups

[Matti Vaittinen]

Hello David & others,

On Tue, Jan 12, 2016 at 04:42:11PM -0500, EXT David Miller wrote:
> But I think your change has an off-by-one bug:
> 
> From: Matti Vaittinen <matti.vaittinen@nokia.com>
> > -		if (id >= mc_groups_longs * BITS_PER_LONG) {
> > +		if (id + n_groups >= mc_groups_longs * BITS_PER_LONG) {
> 
> I think this needs to be "id + n_groups > ".  Consider the existing,
> working, case of "n_groups == 1".  Now you're adding '1' and therefore
> the test needs to be adjusted from >= to >.

Absolutely. I did patch v2.

Br.
    Matti Vaittinen