All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] RFC: tipc int vs size_t fixes
@ 2010-10-27 19:29 Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size() Paul Gortmaker
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:29 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security


[apologies if you get this 2x ; a bogus Status: header caused them
 to fail vger's sanity check for netdev, so resending.]

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size()
  2010-10-27 19:29 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker
@ 2010-10-27 19:29 ` Paul Gortmaker
  2010-10-28 14:07   ` Neil Horman
  2010-10-27 19:29 ` [PATCH 2/4] tipc: Fix bugs in tipc_msg_build() Paul Gortmaker
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:29 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/4] tipc: Fix bugs in tipc_msg_build()
  2010-10-27 19:29 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size() Paul Gortmaker
@ 2010-10-27 19:29 ` Paul Gortmaker
  2010-10-28 14:19   ` Neil Horman
  2010-10-27 19:29 ` [PATCH 3/4] tipc: Update arguments to use size_t for iovec array sizes Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 4/4] tipc: Fix bugs in sending of large amounts of byte-stream data Paul Gortmaker
  3 siblings, 1 reply; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:29 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

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

Enhances TIPC's creation of message buffers so that it works properly
when large amounts of data are involved. Calculations are now done
using "size_t" where needed, and comparisons no longer mix signed and
unsigned size values.

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

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 38360a9..b67e831 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -96,27 +96,36 @@ size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect)
  *
  * Note: Caller must not hold any locks in case copy_from_user() is interrupted!
  *
- * Returns message data size or errno
+ * If successful, creates message buffer and returns message data size
+ * (which must be <= TIPC_MAX_USER_MSG_SIZE).
+ * If fails only because message data size exceeds the specified maximum
+ * returns message data size, but doesn't created a message buffer.
+ * If fails for any other reason returns errno and doesn't create a buffer.
  */
 
 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)
+		   struct iovec const *msg_sect, size_t num_sect,
+		   u32 max_size, int usrmem, struct sk_buff **buf)
 {
-	int dsz, sz, hsz, pos, res, cnt;
+	size_t dsz;
+	u32 hsz;
+	u32 sz;
+	size_t pos;
+	size_t cnt;
+	int res;
 
 	dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
-	if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
+	if (unlikely(dsz > (size_t)TIPC_MAX_USER_MSG_SIZE)) {
 		*buf = NULL;
 		return -EINVAL;
 	}
 
 	pos = hsz = msg_hdr_sz(hdr);
-	sz = hsz + dsz;
+	sz = hsz + (u32)dsz;
 	msg_set_size(hdr, sz);
 	if (unlikely(sz > max_size)) {
 		*buf = NULL;
-		return dsz;
+		return (int)dsz;
 	}
 
 	*buf = tipc_buf_acquire(sz);
@@ -135,7 +144,7 @@ int tipc_msg_build(struct tipc_msg *hdr,
 		pos += msg_sect[cnt].iov_len;
 	}
 	if (likely(res))
-		return dsz;
+		return (int)dsz;
 
 	buf_discard(*buf);
 	*buf = NULL;
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index a132800..41fb532 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -713,8 +713,8 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
 			    u32 hsize, u32 destnode);
 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);
+		   struct iovec const *msg_sect, size_t num_sect,
+		   u32 max_size, int usrmem, struct sk_buff **buf);
 
 static inline void msg_set_media_addr(struct tipc_msg *m, struct tipc_media_addr *a)
 {
-- 
1.7.1.GIT


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/4] tipc: Update arguments to use size_t for iovec array sizes
  2010-10-27 19:29 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size() Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 2/4] tipc: Fix bugs in tipc_msg_build() Paul Gortmaker
@ 2010-10-27 19:29 ` Paul Gortmaker
  2010-10-27 19:29 ` [PATCH 4/4] tipc: Fix bugs in sending of large amounts of byte-stream data Paul Gortmaker
  3 siblings, 0 replies; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:29 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

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

Ensures that all routines that pass iovec arrays as arguments specify
the array length using "size_t" to avoid the risk of accidentally
truncating a large array.

Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
---
 include/net/tipc/tipc.h |    8 ++++----
 net/tipc/link.c         |    6 +++---
 net/tipc/link.h         |    2 +-
 net/tipc/port.c         |   16 ++++++++--------
 net/tipc/port.h         |    2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/net/tipc/tipc.h b/include/net/tipc/tipc.h
index 1e0645e..7750e2b 100644
--- a/include/net/tipc/tipc.h
+++ b/include/net/tipc/tipc.h
@@ -157,18 +157,18 @@ int tipc_shutdown(u32 ref);
 
 
 int tipc_send(u32 portref,
-	      unsigned int num_sect,
+	      size_t num_sect,
 	      struct iovec const *msg_sect);
 
 int tipc_send2name(u32 portref, 
 		   struct tipc_name const *name, 
 		   u32 domain,
-		   unsigned int num_sect,
+		   size_t num_sect,
 		   struct iovec const *msg_sect);
 
 int tipc_send2port(u32 portref,
 		   struct tipc_portid const *dest,
-		   unsigned int num_sect,
+		   size_t num_sect,
 		   struct iovec const *msg_sect);
 
 int tipc_send_buf2port(u32 portref,
@@ -179,7 +179,7 @@ int tipc_send_buf2port(u32 portref,
 int tipc_multicast(u32 portref, 
 		   struct tipc_name_seq const *seq, 
 		   u32 domain,	/* currently unused */
-		   unsigned int section_count,
+		   size_t section_count,
 		   struct iovec const *msg);
 #endif
 
diff --git a/net/tipc/link.c b/net/tipc/link.c
index a997d9f..a92099f 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -106,7 +106,7 @@ static int  link_recv_changeover_msg(struct link **l_ptr, struct sk_buff **buf);
 static void link_set_supervision_props(struct link *l_ptr, u32 tolerance);
 static int  link_send_sections_long(struct port *sender,
 				    struct iovec const *msg_sect,
-				    u32 num_sect, u32 destnode);
+				    size_t num_sect, u32 destnode);
 static void link_check_defragm_bufs(struct link *l_ptr);
 static void link_state_event(struct link *l_ptr, u32 event);
 static void link_reset_statistics(struct link *l_ptr);
@@ -1180,7 +1180,7 @@ int tipc_send_buf_fast(struct sk_buff *buf, u32 destnode)
  */
 int tipc_link_send_sections_fast(struct port *sender,
 				 struct iovec const *msg_sect,
-				 const u32 num_sect,
+				 const size_t num_sect,
 				 u32 destaddr)
 {
 	struct tipc_msg *hdr = &sender->publ.phdr;
@@ -1276,7 +1276,7 @@ exit:
  */
 static int link_send_sections_long(struct port *sender,
 				   struct iovec const *msg_sect,
-				   u32 num_sect,
+				   size_t num_sect,
 				   u32 destaddr)
 {
 	struct link *l_ptr;
diff --git a/net/tipc/link.h b/net/tipc/link.h
index f98bc61..8832c78 100644
--- a/net/tipc/link.h
+++ b/net/tipc/link.h
@@ -236,7 +236,7 @@ int tipc_link_send_buf(struct link *l_ptr, struct sk_buff *buf);
 u32 tipc_link_get_max_pkt(u32 dest,u32 selector);
 int tipc_link_send_sections_fast(struct port* sender,
 				 struct iovec const *msg_sect,
-				 const u32 num_sect,
+				 const size_t num_sect,
 				 u32 destnode);
 void tipc_link_recv_bundle(struct sk_buff *buf);
 int  tipc_link_recv_fragment(struct sk_buff **pending,
diff --git a/net/tipc/port.c b/net/tipc/port.c
index 82092ea..c65409b 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -95,7 +95,7 @@ static void port_incr_out_seqno(struct port *p_ptr)
  */
 
 int tipc_multicast(u32 ref, struct tipc_name_seq const *seq, u32 domain,
-		   u32 num_sect, struct iovec const *msg_sect)
+		   size_t num_sect, struct iovec const *msg_sect)
 {
 	struct tipc_msg *hdr;
 	struct sk_buff *buf;
@@ -446,7 +446,7 @@ int tipc_reject_msg(struct sk_buff *buf, u32 err)
 }
 
 int tipc_port_reject_sections(struct port *p_ptr, struct tipc_msg *hdr,
-			      struct iovec const *msg_sect, u32 num_sect,
+			      struct iovec const *msg_sect, size_t num_sect,
 			      int err)
 {
 	struct sk_buff *buf;
@@ -1219,7 +1219,7 @@ int tipc_shutdown(u32 ref)
  *                        message for this node.
  */
 
-static int tipc_port_recv_sections(struct port *sender, unsigned int num_sect,
+static int tipc_port_recv_sections(struct port *sender, size_t num_sect,
 				   struct iovec const *msg_sect)
 {
 	struct sk_buff *buf;
@@ -1236,7 +1236,7 @@ static int tipc_port_recv_sections(struct port *sender, unsigned int num_sect,
  * tipc_send - send message sections on connection
  */
 
-int tipc_send(u32 ref, unsigned int num_sect, struct iovec const *msg_sect)
+int tipc_send(u32 ref, size_t num_sect, struct iovec const *msg_sect)
 {
 	struct port *p_ptr;
 	u32 destnode;
@@ -1277,7 +1277,7 @@ int tipc_send(u32 ref, unsigned int num_sect, struct iovec const *msg_sect)
 static int tipc_forward2name(u32 ref,
 			     struct tipc_name const *name,
 			     u32 domain,
-			     u32 num_sect,
+			     size_t num_sect,
 			     struct iovec const *msg_sect,
 			     struct tipc_portid const *orig,
 			     unsigned int importance)
@@ -1331,7 +1331,7 @@ static int tipc_forward2name(u32 ref,
 int tipc_send2name(u32 ref,
 		   struct tipc_name const *name,
 		   unsigned int domain,
-		   unsigned int num_sect,
+		   size_t num_sect,
 		   struct iovec const *msg_sect)
 {
 	struct tipc_portid orig;
@@ -1348,7 +1348,7 @@ int tipc_send2name(u32 ref,
 
 static int tipc_forward2port(u32 ref,
 			     struct tipc_portid const *dest,
-			     unsigned int num_sect,
+			     size_t num_sect,
 			     struct iovec const *msg_sect,
 			     struct tipc_portid const *orig,
 			     unsigned int importance)
@@ -1389,7 +1389,7 @@ static int tipc_forward2port(u32 ref,
 
 int tipc_send2port(u32 ref,
 		   struct tipc_portid const *dest,
-		   unsigned int num_sect,
+		   size_t num_sect,
 		   struct iovec const *msg_sect)
 {
 	struct tipc_portid orig;
diff --git a/net/tipc/port.h b/net/tipc/port.h
index 73bbf44..baa2e71 100644
--- a/net/tipc/port.h
+++ b/net/tipc/port.h
@@ -110,7 +110,7 @@ extern spinlock_t tipc_port_list_lock;
 struct port_list;
 
 int tipc_port_reject_sections(struct port *p_ptr, struct tipc_msg *hdr,
-			      struct iovec const *msg_sect, u32 num_sect,
+			      struct iovec const *msg_sect, size_t num_sect,
 			      int err);
 struct sk_buff *tipc_port_get_ports(void);
 struct sk_buff *port_show_stats(const void *req_tlv_area, int req_tlv_space);
-- 
1.7.1.GIT


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/4] tipc: Fix bugs in sending of large amounts of byte-stream data
  2010-10-27 19:29 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker
                   ` (2 preceding siblings ...)
  2010-10-27 19:29 ` [PATCH 3/4] tipc: Update arguments to use size_t for iovec array sizes Paul Gortmaker
@ 2010-10-27 19:29 ` Paul Gortmaker
  3 siblings, 0 replies; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:29 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

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

Enhances the routine that sends data for SOCK_STREAM sockets to
fix the following issues:

- Uses "size_t" to specify the size and number of iovec array entries
  to avoid the risk of accidentally truncating data.
- No longer uses comparisons that mix signed and unsigned size values.
- Adds check to terminate sending early if continuation would require
  returning a value too large to fit in an "int".

Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
---
 net/tipc/socket.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 33217fc..081eda5 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -703,12 +703,12 @@ static int send_stream(struct kiocb *iocb, struct socket *sock,
 	struct msghdr my_msg;
 	struct iovec my_iov;
 	struct iovec *curr_iov;
-	int curr_iovlen;
+	size_t curr_iovlen;
 	char __user *curr_start;
 	u32 hdr_size;
-	int curr_left;
-	int bytes_to_send;
-	int bytes_sent;
+	size_t curr_left;
+	size_t bytes_to_send;
+	size_t bytes_sent;
 	int res;
 
 	lock_sock(sk);
@@ -757,15 +757,19 @@ static int send_stream(struct kiocb *iocb, struct socket *sock,
 
 		while (curr_left) {
 			bytes_to_send = tport->max_pkt - hdr_size;
-			if (bytes_to_send > TIPC_MAX_USER_MSG_SIZE)
-				bytes_to_send = TIPC_MAX_USER_MSG_SIZE;
+			if (bytes_to_send > (size_t)TIPC_MAX_USER_MSG_SIZE)
+				bytes_to_send = (size_t)TIPC_MAX_USER_MSG_SIZE;
 			if (curr_left < bytes_to_send)
 				bytes_to_send = curr_left;
+			if (bytes_to_send > (size_t)INT_MAX - bytes_sent) {
+				res = (int)bytes_sent;
+				goto exit;
+			}
 			my_iov.iov_base = curr_start;
 			my_iov.iov_len = bytes_to_send;
 			if ((res = send_packet(NULL, sock, &my_msg, 0)) < 0) {
 				if (bytes_sent)
-					res = bytes_sent;
+					res = (int)bytes_sent;
 				goto exit;
 			}
 			curr_left -= bytes_to_send;
@@ -775,7 +779,7 @@ static int send_stream(struct kiocb *iocb, struct socket *sock,
 
 		curr_iov++;
 	}
-	res = bytes_sent;
+	res = (int)bytes_sent;
 exit:
 	release_sock(sk);
 	return res;
-- 
1.7.1.GIT


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size()
  2010-10-27 19:29 ` [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size() Paul Gortmaker
@ 2010-10-28 14:07   ` Neil Horman
  0 siblings, 0 replies; 8+ messages in thread
From: Neil Horman @ 2010-10-28 14:07 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: davem, netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/4] tipc: Fix bugs in tipc_msg_build()
  2010-10-27 19:29 ` [PATCH 2/4] tipc: Fix bugs in tipc_msg_build() Paul Gortmaker
@ 2010-10-28 14:19   ` Neil Horman
  0 siblings, 0 replies; 8+ messages in thread
From: Neil Horman @ 2010-10-28 14:19 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: davem, netdev, allan.stephens, drosenberg, jon.maloy, torvalds, security

On Wed, Oct 27, 2010 at 03:29:31PM -0400, Paul Gortmaker wrote:
> From: Allan Stephens <Allan.Stephens@windriver.com>
> 
> Enhances TIPC's creation of message buffers so that it works properly
> when large amounts of data are involved. Calculations are now done
> using "size_t" where needed, and comparisons no longer mix signed and
> unsigned size values.
> 
> Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
> ---
>  net/tipc/msg.c |   25 +++++++++++++++++--------
>  net/tipc/msg.h |    4 ++--
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 38360a9..b67e831 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -96,27 +96,36 @@ size_t tipc_msg_calc_data_size(struct iovec const *msg_sect, size_t num_sect)
>   *
>   * Note: Caller must not hold any locks in case copy_from_user() is interrupted!
>   *
> - * Returns message data size or errno
> + * If successful, creates message buffer and returns message data size
> + * (which must be <= TIPC_MAX_USER_MSG_SIZE).
> + * If fails only because message data size exceeds the specified maximum
> + * returns message data size, but doesn't created a message buffer.
> + * If fails for any other reason returns errno and doesn't create a buffer.
>   */
>  
>  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)
> +		   struct iovec const *msg_sect, size_t num_sect,
> +		   u32 max_size, int usrmem, struct sk_buff **buf)
>  {
> -	int dsz, sz, hsz, pos, res, cnt;
> +	size_t dsz;
> +	u32 hsz;
> +	u32 sz;
> +	size_t pos;
> +	size_t cnt;
> +	int res;
>  
>  	dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
> -	if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
> +	if (unlikely(dsz > (size_t)TIPC_MAX_USER_MSG_SIZE)) {
>  		*buf = NULL;
>  		return -EINVAL;
>  	}
>  
>  	pos = hsz = msg_hdr_sz(hdr);
> -	sz = hsz + dsz;
> +	sz = hsz + (u32)dsz;
>  	msg_set_size(hdr, sz);
>  	if (unlikely(sz > max_size)) {
>  		*buf = NULL;
> -		return dsz;
> +		return (int)dsz;
Don't you want to return -ETOOBIG here, or something simmilar?  All the call
chains that I see for tipc_msg_build check for negative return codes and check
those against specific values.  Why return some random too-big-value?

>  	}
>  
>  	*buf = tipc_buf_acquire(sz);
> @@ -135,7 +144,7 @@ int tipc_msg_build(struct tipc_msg *hdr,
>  		pos += msg_sect[cnt].iov_len;
>  	}
>  	if (likely(res))
> -		return dsz;
> +		return (int)dsz;
>  
Ditto the above

>  	buf_discard(*buf);
>  	*buf = NULL;
> diff --git a/net/tipc/msg.h b/net/tipc/msg.h
> index a132800..41fb532 100644
> --- a/net/tipc/msg.h
> +++ b/net/tipc/msg.h
> @@ -713,8 +713,8 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>  			    u32 hsize, u32 destnode);
>  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);
> +		   struct iovec const *msg_sect, size_t num_sect,
> +		   u32 max_size, int usrmem, struct sk_buff **buf);
>  
>  static inline void msg_set_media_addr(struct tipc_msg *m, struct tipc_media_addr *a)
>  {

I count 3 other callers of tipc_msg_calc_data_size (tipc_send,
tipc_forward2name, tipc_forward2port).  Where are the updates to those functions
to make the corresponding size_t adjustments
Neil

> -- 
> 1.7.1.GIT
> 
> --
> 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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 0/4] RFC: tipc int vs size_t fixes
@ 2010-10-27 19:13 Paul Gortmaker
  0 siblings, 0 replies; 8+ messages in thread
From: Paul Gortmaker @ 2010-10-27 19:13 UTC (permalink / raw)
  To: davem; +Cc: netdev, allan.stephens, drosenberg, jon.maloy, security


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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2010-10-28 14:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-27 19:29 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker
2010-10-27 19:29 ` [PATCH 1/4] tipc: Fix bugs in tipc_msg_calc_data_size() Paul Gortmaker
2010-10-28 14:07   ` Neil Horman
2010-10-27 19:29 ` [PATCH 2/4] tipc: Fix bugs in tipc_msg_build() Paul Gortmaker
2010-10-28 14:19   ` Neil Horman
2010-10-27 19:29 ` [PATCH 3/4] tipc: Update arguments to use size_t for iovec array sizes Paul Gortmaker
2010-10-27 19:29 ` [PATCH 4/4] tipc: Fix bugs in sending of large amounts of byte-stream data Paul Gortmaker
  -- strict thread matches above, loose matches on Subject: below --
2010-10-27 19:13 [PATCH 0/4] RFC: tipc int vs size_t fixes Paul Gortmaker

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.