| Message ID | 1290011836.26402.37.camel@dan |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Dan Rosenberg <drosenberg@vsecurity.com> Date: Wed, 17 Nov 2010 11:37:16 -0500 > In rds_cmsg_rdma_args(), the user-provided args->nr_local value is > restricted to less than UINT_MAX. This seems to need a tighter upper > bound, since the calculation of total iov_size can overflow, resulting > in a small sock_kmalloc() allocation. This would probably just result > in walking off the heap and crashing when calling rds_rdma_pages() with > a high count value. If it somehow doesn't crash here, then memory > corruption could occur soon after. > > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> Applied, thanks Dan. -- 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, Nov 17, 2010 at 9:52 AM, David Miller <davem@davemloft.net> wrote: > > Applied, thanks Dan. Why is the cast there? It looks really odd. And it's definitely pointless, since nr_local is already "uint64_t". If the issue is that people don't want to worry about the signedness of nr_local (which isn't obvious in the local scope), then I think it would be _much_ better to write the code as if (args->nr_local < 0 || args->nr_local >UIO_MAXIOV) than to have an illogical cast in there. Maybe we should add a helper function ("in_range()" or whatever) that does this. IOW, maybe something like if (!in_range(args->nr_local, 0, UID_MAXIOV)) would be nicer? With some appropriate macro magic to make it all type-safe? 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
From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Nov 2010 12:07:17 -0800 > Why is the cast there? It looks really odd. And it's definitely > pointless, since nr_local is already "uint64_t". It looks pointless to me too. I'll remove it. Thanks. -- 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
> > Why is the cast there? It looks really odd. And it's definitely > pointless, since nr_local is already "uint64_t". > I agree - I was just imitating the original code. Of course feel free to alter my patch as you wish. -Dan -- 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/rds/rdma.c b/net/rds/rdma.c index 8920f2a..0a969f6 100644 --- a/net/rds/rdma.c +++ b/net/rds/rdma.c @@ -567,7 +567,7 @@ int rds_cmsg_rdma_args(struct rds_sock *rs, struct rds_message *rm, goto out; } - if (args->nr_local > (u64)UINT_MAX) { + if (args->nr_local > (u64)UIO_MAXIOV) { ret = -EMSGSIZE; goto out; }
In rds_cmsg_rdma_args(), the user-provided args->nr_local value is restricted to less than UINT_MAX. This seems to need a tighter upper bound, since the calculation of total iov_size can overflow, resulting in a small sock_kmalloc() allocation. This would probably just result in walking off the heap and crashing when calling rds_rdma_pages() with a high count value. If it somehow doesn't crash here, then memory corruption could occur soon after. Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> --- net/rds/rdma.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) -- 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