netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).