[PATCH] genetlink: fix unsigned int comparison with less than zero

[PATCH] genetlink: fix unsigned int comparison with less than zero

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

family->id is unsigned, so the less than zero check for
failure return from idr_alloc is never true and so the error exit
is never handled.  Instead, assign err and check if this is less
than zero since this is a signed integer.

Issue found with static analysis with CoverityScan, CID 1375916

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 net/netlink/genetlink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f0b65fe..2ea61ba 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -360,12 +360,10 @@ int genl_register_family(struct genl_family *family)
 	} else
 		family->attrbuf = NULL;
 
-	family->id = idr_alloc(&genl_fam_idr, family,
+	family->id = err = idr_alloc(&genl_fam_idr, family,
 			       start, end + 1, GFP_KERNEL);
-	if (family->id < 0) {
-		err = family->id;
+	if (err < 0)
 		goto errout_free;
-	}
 
 	err = genl_validate_assign_mc_groups(family);
 	if (err)
-- 
2.10.2

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[Cong Wang]

On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> family->id is unsigned, so the less than zero check for
> failure return from idr_alloc is never true and so the error exit
> is never handled.  Instead, assign err and check if this is less
> than zero since this is a signed integer.

Why family->id can't be just signed int? For me it should be.

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[Johannes Berg]

On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote:
> On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com
> > wrote:
> > 
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > family->id is unsigned, so the less than zero check for
> > failure return from idr_alloc is never true and so the error exit
> > is never handled.  Instead, assign err and check if this is less
> > than zero since this is a signed integer.
> 
> Why family->id can't be just signed int? For me it should be.

I suppose it could be, since family IDs are allocated in a 16-bit range
anyway. But family IDs can also never actually be negative, so having
an unsigned int in the struct makes sense too.

I tend to think this patch is fine.

johannes

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[Cong Wang]

On Sat, Nov 12, 2016 at 1:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote:
>> On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com
>> > wrote:
>> >
>> > From: Colin Ian King <colin.king@canonical.com>
>> >
>> > family->id is unsigned, so the less than zero check for
>> > failure return from idr_alloc is never true and so the error exit
>> > is never handled.  Instead, assign err and check if this is less
>> > than zero since this is a signed integer.
>>
>> Why family->id can't be just signed int? For me it should be.
>
> I suppose it could be, since family IDs are allocated in a 16-bit range
> anyway. But family IDs can also never actually be negative, so having
> an unsigned int in the struct makes sense too.

All idr_* API's accept int, rather than unsigned int. This is my point.

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[Johannes Berg]

> > I suppose it could be, since family IDs are allocated in a 16-bit
> > range
> > anyway. But family IDs can also never actually be negative, so
> > having
> > an unsigned int in the struct makes sense too.
> 
> All idr_* API's accept int, rather than unsigned int. This is my
> point.

Sure, but that's an internal implementation detail. The struct
genl_family is also an external API towards its users, and there
negative numbers make no sense whatsoever.

johannes

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[David Miller]

From: Colin King <colin.king@canonical.com>
Date: Thu, 10 Nov 2016 15:57:58 +0000

> From: Colin Ian King <colin.king@canonical.com>
> 
> family->id is unsigned, so the less than zero check for
> failure return from idr_alloc is never true and so the error exit
> is never handled.  Instead, assign err and check if this is less
> than zero since this is a signed integer.
> 
> Issue found with static analysis with CoverityScan, CID 1375916
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  net/netlink/genetlink.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index f0b65fe..2ea61ba 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -360,12 +360,10 @@ int genl_register_family(struct genl_family *family)
>  	} else
>  		family->attrbuf = NULL;
>  
> -	family->id = idr_alloc(&genl_fam_idr, family,
> +	family->id = err = idr_alloc(&genl_fam_idr, family,
>  			       start, end + 1, GFP_KERNEL);

First of all, if we make this change you must fixup the indentation of
the second l ine of this idr_alloc() call.  Arguments spanning
multiple lines of a call must be indented precisely to the column
following the openning parenthesis of the first line.

Next, the IDR helpers never give us values that fall within the
positive range of an integer that does not fall within the postive
range of an unsigned integer.

We are going to pass this value in later to release the ID, again
the interface will expect a signed rather than an unsigned int.

Therefore is makes sesne only to change the family->id type to
what it must be, which is a signed int.

I've commited the following to net-next:

====================
[PATCH] genetlink: Make family a signed integer.

The idr_alloc(), idr_remove(), et al. routines all expect IDs to be
signed integers.  Therefore make the genl_family member 'id' signed
too.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/net/genetlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 3ec87ba..a34275b 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -48,7 +48,7 @@ struct genl_info;
  * @n_ops: number of operations supported by this family
  */
 struct genl_family {
-	unsigned int		id;		/* private */
+	int			id;		/* private */
 	unsigned int		hdrsize;
 	char			name[GENL_NAMSIZ];
 	unsigned int		version;
-- 
2.7.4

Re: [PATCH] genetlink: fix unsigned int comparison with less than zero

[Cong Wang]

On Sun, Nov 13, 2016 at 9:15 AM, David Miller <davem@davemloft.net> wrote:
> I've commited the following to net-next:
>
> ====================
> [PATCH] genetlink: Make family a signed integer.
>
> The idr_alloc(), idr_remove(), et al. routines all expect IDs to be
> signed integers.  Therefore make the genl_family member 'id' signed
> too.

This is exactly what I replied to Johannes.

Thanks for the fix!