| Message ID | 20180328073513.5294-1-roland@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Mar 28, 2018 at 12:35:13AM -0700, Roland Dreier wrote: > From: Roland Dreier <roland@purestorage.com> > > If userspace passes a sockaddr with sa_family == AF_IB to the ucma join IP > multicast command, the kernel will memcpy() past the end of its buffer. Fix > this by checking the addr_size we get back against the sockaddr buffer sizes. > > Reported-and-tested-by: syzbot+6800425d54ed3ed8135d@syzkaller.appspotmail.com > Signed-off-by: Roland Dreier <roland@purestorage.com> > --- > drivers/infiniband/core/ucma.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index e5a1e7d81326..565efa8f43a5 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -1427,7 +1427,9 @@ static ssize_t ucma_join_ip_multicast(struct ucma_file *file, > join_cmd.uid = cmd.uid; > join_cmd.id = cmd.id; > join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr); > - if (!join_cmd.addr_size) > + if (!join_cmd.addr_size || > + join_cmd.addr_size > sizeof(join_cmd.addr) || > + join_cmd.addr_size > sizeof(cmd.addr)) The "join_cmd.addr_size > sizeof(cmd.addr)" is not needed, because we copy only join_cmd.addr_size bytes and ensure that it has size equal to sizeof(cmd.addr). Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
>> --- a/drivers/infiniband/core/ucma.c >> +++ b/drivers/infiniband/core/ucma.c >> @@ -1427,7 +1427,9 @@ static ssize_t ucma_join_ip_multicast(struct ucma_file *file, >> join_cmd.uid = cmd.uid; >> join_cmd.id = cmd.id; >> join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr); >> - if (!join_cmd.addr_size) >> + if (!join_cmd.addr_size || >> + join_cmd.addr_size > sizeof(join_cmd.addr) || >> + join_cmd.addr_size > sizeof(cmd.addr)) > > The "join_cmd.addr_size > sizeof(cmd.addr)" is not needed, because we > copy only join_cmd.addr_size bytes and ensure that it has size equal to > sizeof(cmd.addr). I don't follow ... what sizes do we ensure are equal? I don't see any tests for equality in the function I'm patching. If anything the test against sizeof(join_cmd.addr) is the one that is superfluous, because we know that struct __kernel_sockaddr_storage (join_cmd.addr member) is at least as big as struct sockaddr_in6 (cmd.addr member). But I thought it was cleaner to check both bounds without relying on knowing the exact struct layouts and types. The compiler will be smart enough to only generate code for the stricter test anyway. Happy to respin if anyone feels differently. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 28, 2018 at 01:13:28AM -0700, Roland Dreier wrote: > >> --- a/drivers/infiniband/core/ucma.c > >> +++ b/drivers/infiniband/core/ucma.c > >> @@ -1427,7 +1427,9 @@ static ssize_t ucma_join_ip_multicast(struct ucma_file *file, > >> join_cmd.uid = cmd.uid; > >> join_cmd.id = cmd.id; > >> join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr); > >> - if (!join_cmd.addr_size) > >> + if (!join_cmd.addr_size || > >> + join_cmd.addr_size > sizeof(join_cmd.addr) || > >> + join_cmd.addr_size > sizeof(cmd.addr)) > > > > The "join_cmd.addr_size > sizeof(cmd.addr)" is not needed, because we > > copy only join_cmd.addr_size bytes and ensure that it has size equal to > > sizeof(cmd.addr). > > I don't follow ... what sizes do we ensure are equal? I don't see any > tests for equality in the function I'm patching. > > If anything the test against sizeof(join_cmd.addr) is the one that is > superfluous, because we know that struct __kernel_sockaddr_storage > (join_cmd.addr member) is at least as big as struct sockaddr_in6 > (cmd.addr member). But I thought it was cleaner to check both bounds > without relying on knowing the exact struct layouts and types. The > compiler will be smart enough to only generate code for the stricter > test anyway. As you wrote, "join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr);" line ensure that join_cmd.addr_size can be 0 and various sizeof(struct sockaddr_i*). It is enough to check that join_cmd.addr_size has enough space to copy join_cmd.addr_size bytes. If you want to ensure that sizeof(cmd.addr) has right size, it is better to add BUILD_BUG_ON(sizeof(cmd.args) > max3(sizeof(struct sockaddr_in), sizeof(struct sockaddr_in6), sizeof(struct sockaddr_ib))) And my claim that "it has size equal to sizeof(cmd.addr)" is misleading, Sorry for that. > > Happy to respin if anyone feels differently. > > - R.
> As you wrote, > "join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr);" > line ensure that join_cmd.addr_size can be 0 and various sizeof(struct > sockaddr_i*). It is enough to check that join_cmd.addr_size has enough > space to copy join_cmd.addr_size bytes. > > If you want to ensure that sizeof(cmd.addr) has right size, it is better to add > BUILD_BUG_ON(sizeof(cmd.args) > max3(sizeof(struct sockaddr_in), sizeof(struct sockaddr_in6), sizeof(struct sockaddr_ib))) I think you may be misunderstanding the bug. Userspace can pass in any family for cmd.addr, but if userspace passes AF_IB into this API, then the memcpy will overrun the buffer because sockaddr_ib is bigger than sockaddr_in6. (That is what syzkbot is reporting) Anyway, let me respin the bug to just check addr_size against min(sizeof(cmd.addr), sizeof(join_cmd.addr)) since I think that will look a little better. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 28, 2018 at 10:17:19AM -0700, Roland Dreier wrote: > > As you wrote, > > "join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr);" > > line ensure that join_cmd.addr_size can be 0 and various sizeof(struct > > sockaddr_i*). It is enough to check that join_cmd.addr_size has enough > > space to copy join_cmd.addr_size bytes. > > > > If you want to ensure that sizeof(cmd.addr) has right size, it is better to add > > BUILD_BUG_ON(sizeof(cmd.args) > max3(sizeof(struct sockaddr_in), sizeof(struct sockaddr_in6), sizeof(struct sockaddr_ib))) > > I think you may be misunderstanding the bug. Userspace can pass in > any family for cmd.addr, but if userspace passes AF_IB into this API, > then the memcpy will overrun the buffer because sockaddr_ib is bigger > than sockaddr_in6. (That is what syzkbot is reporting) > > Anyway, let me respin the bug to just check addr_size against > min(sizeof(cmd.addr), sizeof(join_cmd.addr)) since I think that will > look a little better. Thanks for the explanation and for respinning. > > - R.
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index e5a1e7d81326..565efa8f43a5 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1427,7 +1427,9 @@ static ssize_t ucma_join_ip_multicast(struct ucma_file *file, join_cmd.uid = cmd.uid; join_cmd.id = cmd.id; join_cmd.addr_size = rdma_addr_size((struct sockaddr *) &cmd.addr); - if (!join_cmd.addr_size) + if (!join_cmd.addr_size || + join_cmd.addr_size > sizeof(join_cmd.addr) || + join_cmd.addr_size > sizeof(cmd.addr)) return -EINVAL; join_cmd.join_flags = RDMA_MC_JOIN_FLAG_FULLMEMBER;