[PATCH 0/4] RFC: tipc int vs size_t fixes

[PATCH 0/4] RFC: tipc int vs size_t fixes

[Paul Gortmaker]

This is a starting point for fixing up the mix of int vs. size_t
usage that Dan spotted in TIPC as being open to possible exploits.

Open questions I had remaining with these patches were whether we could
trim out some of the unrequired casts, and whether we wanted to use a
size_t for everything that at any time was based on or compared to
an iov_len, including all instances of num_sect, when things like
iov_length() in <linux/uio.h> use unsigned long for looping over segments.
Also, whether we should give up at INT_MAX or LONG_MAX...

Please suggest changes as required, and I'll integrate them into this
pass1 of draft of patches from Al and resend as required until we've
got a final acceptable solution.

Thanks,
Paul.

Re: [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size()

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Wed, 27 Oct 2010 15:13:33 -0400

> From: Allan Stephens <Allan.Stephens@windriver.com>
> 
> Enhances TIPC's computation of the amount of data to be sent so that
> it works properly when large values are involved. Calculations are now
> done using "size_t" instead of "int", and a check has been added to
> handle cases where the total amount of data exceeds the range of "size_t".
> 
> Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>

The protocol socket I/O call ops can't even return anything larger
than an 'int' because of the signature for those function pointers
(check out *sendmsg and *recvmsg in include/linux/net.h).

So returning "long" from here doesn't make any sense.

You really have to limit the usable lengths to the range of
an 'int' all the way up to the code in net/socket.c

[PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size()

[Paul Gortmaker]

From: Allan Stephens <Allan.Stephens@windriver.com>

Enhances TIPC's computation of the amount of data to be sent so that
it works properly when large values are involved. Calculations are now
done using "size_t" instead of "int", and a check has been added to
handle cases where the total amount of data exceeds the range of "size_t".

Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
---
 net/tipc/msg.c |   17 ++++++++++++-----
 net/tipc/msg.h |    2 +-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index ecb532f..38360a9 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -72,15 +72,22 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
 
 /**
  * tipc_msg_calc_data_size - determine total data size for message
+ *
+ * Note: If total exceeds range of size_t returns largest possible value
  */
 
-int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
+size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect)
 {
-	int dsz = 0;
-	int i;
+	size_t dsz = 0;
+	size_t len;
+	size_t i;
 
-	for (i = 0; i < num_sect; i++)
-		dsz += msg_sect[i].iov_len;
+	for (i = 0; i < num_sect; i++) {
+		len = msg_sect[i].iov_len;
+		if (len > (size_t)LONG_MAX - dsz)
+			return (size_t)LONG_MAX;
+		dsz += len;
+	}
 	return dsz;
 }
 
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 031aad1..a132800 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -711,7 +711,7 @@ static inline void msg_set_dataoctet(struct tipc_msg *m, u32 pos)
 u32 tipc_msg_tot_importance(struct tipc_msg *m);
 void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
 			    u32 hsize, u32 destnode);
-int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect);
+size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect);
 int tipc_msg_build(struct tipc_msg *hdr,
 			    struct iovec const *msg_sect, u32 num_sect,
 			    int max_size, int usrmem, struct sk_buff** buf);
-- 
1.7.1.GIT

Re: [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size()

[Neil Horman]

On Wed, Oct 27, 2010 at 03:29:30PM -0400, Paul Gortmaker wrote:
> From: Allan Stephens <Allan.Stephens@windriver.com>
> 
> Enhances TIPC's computation of the amount of data to be sent so that
> it works properly when large values are involved. Calculations are now
> done using "size_t" instead of "int", and a check has been added to
> handle cases where the total amount of data exceeds the range of "size_t".
> 
> Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
> ---
>  net/tipc/msg.c |   17 ++++++++++++-----
>  net/tipc/msg.h |    2 +-
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index ecb532f..38360a9 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -72,15 +72,22 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>  
>  /**
>   * tipc_msg_calc_data_size - determine total data size for message
> + *
> + * Note: If total exceeds range of size_t returns largest possible value
>   */
>  
> -int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
> +size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect)
>  {
> -	int dsz = 0;
> -	int i;
> +	size_t dsz = 0;
> +	size_t len;
> +	size_t i;
>  
> -	for (i = 0; i < num_sect; i++)
> -		dsz += msg_sect[i].iov_len;
> +	for (i = 0; i < num_sect; i++) {
> +		len = msg_sect[i].iov_len;
> +		if (len > (size_t)LONG_MAX - dsz)
> +			return (size_t)LONG_MAX;
> +		dsz += len;
> +	}
>  	return dsz;
>  }
>  
> diff --git a/net/tipc/msg.h b/net/tipc/msg.h
> index 031aad1..a132800 100644
> --- a/net/tipc/msg.h
> +++ b/net/tipc/msg.h
> @@ -711,7 +711,7 @@ static inline void msg_set_dataoctet(struct tipc_msg *m, u32 pos)
>  u32 tipc_msg_tot_importance(struct tipc_msg *m);
>  void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>  			    u32 hsize, u32 destnode);
> -int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect);
> +size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect);
>  int tipc_msg_build(struct tipc_msg *hdr,
>  			    struct iovec const *msg_sect, u32 num_sect,
>  			    int max_size, int usrmem, struct sk_buff** buf);
> -- 
> 1.7.1.GIT
> 
You should probably roll this patch together with your second one.  The caller
tipc_msg_build will otherwise throw a warning when you build, as it assigns the
return value with this patch (a size_t) to a signed integer.  It probably won't
matter since you limit dsz to TIPC_MAX_USER_MSG_SIZE which is small in
comparison to the size of an int, but nevertheless, the two patches are related,
so you can merge them.

Neil

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