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.
Attachment:
signature.asc
Description: PGP signature
