| Message ID | 20101027.105047.183059900.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
The proposed fix is a start, but it's not sufficient to completely fix the problem. What if the total of the iovecs wraps around back to 0? The total size will be returned as a small number, but large amounts of data will be copied into the allocated buffer since the individual iovecs can have arbitrary sizes. -Dan On Wed, 2010-10-27 at 10:50 -0700, David Miller wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > Date: Wed, 27 Oct 2010 10:37:46 -0700 > > > If you _really_ care deeply, then some packet-oriented protocol can > > just have its own private packet size limit (which would be way less > > than 2GB), and then just look at the total size and say "oh, the total > > size is bigger than my limit, so I'll just error out". Then, the fact > > that verify_iovec() may have truncated the message to 2GB-1 doesn't > > matter at all. > > > > (Practically speaking, I bet all packet-oriented protocols already > > have a limit that is enforced by simply allocation patterns, so I > > don't think it's actually a problem even now) > > This is, as it turns out, effectively what the TIPC socket layer > already does. > > Most of the send calls that propagate down to this code adding up the > iov_len lengths gets passed a maximum packet size. > > Anyways, here is what I came up with to kill this specific bug in > TIPC: > > From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001 > From: David S. Miller <davem@davemloft.net> > Date: Wed, 27 Oct 2010 10:46:59 -0700 > Subject: [PATCH] tipc: Fix iov_len handling in message send path. > > Use size_t to add together iov_len's from the iovec, error > out with -EMSGSIZE if total is greater than INT_MAX. > > Reported-by: Dan Rosenberg <drosenberg@vsecurity.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > net/tipc/msg.c | 13 ++++++++++--- > 1 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c > index ecb532f..b29eb8b 100644 > --- a/net/tipc/msg.c > +++ b/net/tipc/msg.c > @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type, > > int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect) > { > - int dsz = 0; > + size_t dsz = 0; > int i; > > for (i = 0; i < num_sect; i++) > dsz += msg_sect[i].iov_len; > + > + if (dsz > INT_MAX) > + return -EMSGSIZE; > + > return dsz; > } > > @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 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, u32 num_sect, > + int max_size, int usrmem, struct sk_buff** buf) > { > int dsz, sz, hsz, pos, res, cnt; > > dsz = tipc_msg_calc_data_size(msg_sect, num_sect); > + if (dsz < 0) > + return dsz; > + > if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) { > *buf = NULL; > return -EINVAL; > -- > 1.7.3.2 -- 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
On Wed, Oct 27, 2010 at 1:50 PM, David Miller <davem@davemloft.net> wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > Date: Wed, 27 Oct 2010 10:37:46 -0700 > >> If you _really_ care deeply, then some packet-oriented protocol can >> just have its own private packet size limit (which would be way less >> than 2GB), and then just look at the total size and say "oh, the total >> size is bigger than my limit, so I'll just error out". Then, the fact >> that verify_iovec() may have truncated the message to 2GB-1 doesn't >> matter at all. >> >> (Practically speaking, I bet all packet-oriented protocols already >> have a limit that is enforced by simply allocation patterns, so I >> don't think it's actually a problem even now) > > This is, as it turns out, effectively what the TIPC socket layer > already does. > > Most of the send calls that propagate down to this code adding up the > iov_len lengths gets passed a maximum packet size. > > Anyways, here is what I came up with to kill this specific bug in > TIPC: > > From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001 > From: David S. Miller <davem@davemloft.net> > Date: Wed, 27 Oct 2010 10:46:59 -0700 > Subject: [PATCH] tipc: Fix iov_len handling in message send path. > > Use size_t to add together iov_len's from the iovec, error > out with -EMSGSIZE if total is greater than INT_MAX. > > Reported-by: Dan Rosenberg <drosenberg@vsecurity.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > net/tipc/msg.c | 13 ++++++++++--- > 1 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c > index ecb532f..b29eb8b 100644 > --- a/net/tipc/msg.c > +++ b/net/tipc/msg.c > @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type, > > int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect) > { > - int dsz = 0; > + size_t dsz = 0; > int i; > > for (i = 0; i < num_sect; i++) > dsz += msg_sect[i].iov_len; > + > + if (dsz > INT_MAX) > + return -EMSGSIZE; > + I've got some patches from Al that I'm pre-reviewing/testing before spamming everyone with them (will send within 24h, if that is OK). Anyway, in the above, does it make sense to check for the overflow incrementally, i.e. something like this? - 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 > LONG_MAX - dsz) + return LONG_MAX; + dsz += len; + } (where len and dsz are both size_t). Thanks, Paul. > return dsz; > } > > @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 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, u32 num_sect, > + int max_size, int usrmem, struct sk_buff** buf) > { > int dsz, sz, hsz, pos, res, cnt; > > dsz = tipc_msg_calc_data_size(msg_sect, num_sect); > + if (dsz < 0) > + return dsz; > + > if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) { > *buf = NULL; > return -EINVAL; > -- > 1.7.3.2 > > -- > 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 > -- 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
From: Dan Rosenberg <drosenberg@vsecurity.com> Date: Wed, 27 Oct 2010 14:26:19 -0400 > The proposed fix is a start, but it's not sufficient to completely fix > the problem. What if the total of the iovecs wraps around back to 0? > The total size will be returned as a small number, but large amounts of > data will be copied into the allocated buffer since the individual > iovecs can have arbitrary sizes. The calculated length total is what should be used by the calling function to decide how much to copy. Sorry, I assumed the TIPC doing was sane like the rest of the networking. :-( I'll fix this up. -- 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
From: Paul Gortmaker <paul.gortmaker@windriver.com> Date: Wed, 27 Oct 2010 14:27:13 -0400 > I've got some patches from Al that I'm pre-reviewing/testing > before spamming everyone with them (will send within 24h, if > that is OK). Anyway, in the above, does it make sense to > check for the overflow incrementally, i.e. something like this? Can you please not work on this in private? That's the only reason we have duplicated work here. -- 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
On Wed, Oct 27, 2010 at 11:26 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote: > The proposed fix is a start, but it's not sufficient to completely fix > the problem. What if the total of the iovecs wraps around back to 0? > The total size will be returned as a small number, but large amounts of > data will be copied into the allocated buffer since the individual > iovecs can have arbitrary sizes. That's why I suggested just fixing iovec_verify() to do this all. It already walks the thing, and while it allows the overflow right now (and only wants to make sure the end result is positive to separate it out from error numbers), that was always a ugly thing to do. And the thing is, once you disallow overflow, you really MUST NOT return an error - you need to instead cap the max, the way I also did in my patch. Why? Because for a streaming thing, it's entirely possible that somebody tries to write out a whole mmap'ed file in one go, for example. Returning an error because somebody tries to write 8GB in one single system call is wrong as it would result in the application reporting an IO error - but saying "I will write out part of it" is fine, and then it's up to the user to loop over it (it already needs to do that for other reasons for partial IO). So doing this in verify_iovec() (and verify_compat_iovec - which I didn't do in my RFC patch) really does fix everything, and means that the individual socket types never have to worry about the subtle cases of overflow in any type. Linus -- 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
On 10-10-27 02:35 PM, David Miller wrote: > From: Paul Gortmaker<paul.gortmaker@windriver.com> > Date: Wed, 27 Oct 2010 14:27:13 -0400 > >> I've got some patches from Al that I'm pre-reviewing/testing >> before spamming everyone with them (will send within 24h, if >> that is OK). Anyway, in the above, does it make sense to >> check for the overflow incrementally, i.e. something like this? > > Can you please not work on this in private? That's the only reason > we have duplicated work here. Sorry -- my intent was to not waste people's time with stuff that might have obvious issues. I'll send what I have now and we can go from there. P. -- 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
From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 27 Oct 2010 11:51:19 -0700 > So doing this in verify_iovec() (and verify_compat_iovec - which I > didn't do in my RFC patch) really does fix everything, and means that > the individual socket types never have to worry about the subtle cases > of overflow in any type. I completely agree about capping things in verify_iovec(). And it was completely erroneous of me to change verify_iovec() to return 'long' instead of 'int', it should have stayed at 'int' with a cap. Because the protocols don't even have a way to return something larger than an 'int' as a return value due to the signature of the recvmsg() and sendmsg() socket ops. -- 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
diff --git a/net/tipc/msg.c b/net/tipc/msg.c index ecb532f..b29eb8b 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type, int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect) { - int dsz = 0; + size_t dsz = 0; int i; for (i = 0; i < num_sect; i++) dsz += msg_sect[i].iov_len; + + if (dsz > INT_MAX) + return -EMSGSIZE; + return dsz; } @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 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, u32 num_sect, + int max_size, int usrmem, struct sk_buff** buf) { int dsz, sz, hsz, pos, res, cnt; dsz = tipc_msg_calc_data_size(msg_sect, num_sect); + if (dsz < 0) + return dsz; + if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) { *buf = NULL; return -EINVAL;