On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
> The svc_addr_len() helper function can return a negative errno value,
> but its return type is size_t, which is unsigned.
>
> The RDMA transport code passes this return value directly to memset(),
> without checking first if it's negative. This could cause memset() to
> clobber a large piece of memory if svc_addr_len() has returned an
> error.
I'd still like to understand when this can happen, to better understand
how the error should be handled.
Also:
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> include/linux/sunrpc/svc_xprt.h | 3 ++-
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++++--
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index 0127dac..c2aa8cd 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
> return ret;
> }
>
> -static inline size_t svc_addr_len(struct sockaddr *sa)
> +static inline int svc_addr_len(const struct sockaddr *sa)
> {
> switch (sa->sa_family) {
> case AF_INET:
> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
> case AF_INET6:
> return sizeof(struct sockaddr_in6);
> }
> +
> return -EAFNOSUPPORT;
> }
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 3d810e7..d1ec6f9 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
> struct svcxprt_rdma *listen_xprt = new_cma_id->context;
> struct svcxprt_rdma *newxprt;
> struct sockaddr *sa;
> + int len;
>
> /* Create a new transport */
> newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
> @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>
> /* Set the local and remote addresses in the transport */
> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> + len = svc_addr_len(sa);
> + if (len < 0) {
> + dprintk("svcrdma: dst_addr has a bad address family\n");
> + return;
we're probably leaking something here.
I don't want to fix this until it's understood well enough to fix it
correctly.
--b.
> + }
> + svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
> +
> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
> - svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> + len = svc_addr_len(sa);
> + if (len < 0) {
> + dprintk("svcrdma: src_addr has a bad address family\n");
> + return;
> + }
> + svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>
> /*
> * Enqueue the new transport on the accept queue of the listening
>
On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>> The svc_addr_len() helper function can return a negative errno value,
>> but its return type is size_t, which is unsigned.
>>
>> The RDMA transport code passes this return value directly to
>> memset(),
>> without checking first if it's negative. This could cause memset()
>> to
>> clobber a large piece of memory if svc_addr_len() has returned an
>> error.
>
> I'd still like to understand when this can happen, to better
> understand
> how the error should be handled.
>
> Also:
>
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> include/linux/sunrpc/svc_xprt.h | 3 ++-
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++++--
>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/
>> svc_xprt.h
>> index 0127dac..c2aa8cd 100644
>> --- a/include/linux/sunrpc/svc_xprt.h
>> +++ b/include/linux/sunrpc/svc_xprt.h
>> @@ -113,7 +113,7 @@ static inline unsigned short
>> svc_addr_port(struct sockaddr *sa)
>> return ret;
>> }
>>
>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>> +static inline int svc_addr_len(const struct sockaddr *sa)
>> {
>> switch (sa->sa_family) {
>> case AF_INET:
>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct
>> sockaddr *sa)
>> case AF_INET6:
>> return sizeof(struct sockaddr_in6);
>> }
>> +
>> return -EAFNOSUPPORT;
>> }
It's clearly a bug to return -EAFNOSUPPORT in a size_t.
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/
>> xprtrdma/svc_rdma_transport.c
>> index 3d810e7..d1ec6f9 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct
>> rdma_cm_id *new_cma_id, size_t client_ird)
>> struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>> struct svcxprt_rdma *newxprt;
>> struct sockaddr *sa;
>> + int len;
>>
>> /* Create a new transport */
>> newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>> rdma_cm_id *new_cma_id, size_t client_ird)
>>
>> /* Set the local and remote addresses in the transport */
>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> + len = svc_addr_len(sa);
>> + if (len < 0) {
>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>> + return;
>
> we're probably leaking something here.
>
> I don't want to fix this until it's understood well enough to fix it
> correctly.
Tom needs respond to this, but I think it would be safe to simply
BUG() here.
> --b.
>
>> + }
>> + svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
>> +
>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
>> - svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> + len = svc_addr_len(sa);
>> + if (len < 0) {
>> + dprintk("svcrdma: src_addr has a bad address family\n");
>> + return;
>> + }
>> + svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>>
>> /*
>> * Enqueue the new transport on the accept queue of the listening
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Mar 24, 2009 at 04:37:49PM -0400, Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>>> The svc_addr_len() helper function can return a negative errno value,
>>> but its return type is size_t, which is unsigned.
>>>
>>> The RDMA transport code passes this return value directly to
>>> memset(),
>>> without checking first if it's negative. This could cause memset()
>>> to
>>> clobber a large piece of memory if svc_addr_len() has returned an
>>> error.
>>
>> I'd still like to understand when this can happen, to better
>> understand
>> how the error should be handled.
>>
>> Also:
>>
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>>
>>> include/linux/sunrpc/svc_xprt.h | 3 ++-
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++++--
>>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/
>>> svc_xprt.h
>>> index 0127dac..c2aa8cd 100644
>>> --- a/include/linux/sunrpc/svc_xprt.h
>>> +++ b/include/linux/sunrpc/svc_xprt.h
>>> @@ -113,7 +113,7 @@ static inline unsigned short
>>> svc_addr_port(struct sockaddr *sa)
>>> return ret;
>>> }
>>>
>>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>>> +static inline int svc_addr_len(const struct sockaddr *sa)
>>> {
>>> switch (sa->sa_family) {
>>> case AF_INET:
>>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct
>>> sockaddr *sa)
>>> case AF_INET6:
>>> return sizeof(struct sockaddr_in6);
>>> }
>>> +
>>> return -EAFNOSUPPORT;
>>> }
>
> It's clearly a bug to return -EAFNOSUPPORT in a size_t.
I agree. My comment is just that this patch doesn't necessarily fix the
real bug; so as long as there's a bug here, I'd prefer it to be an
obvious one.
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/
>>> xprtrdma/svc_rdma_transport.c
>>> index 3d810e7..d1ec6f9 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id
>>> *new_cma_id, size_t client_ird)
>>> struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>>> struct svcxprt_rdma *newxprt;
>>> struct sockaddr *sa;
>>> + int len;
>>>
>>> /* Create a new transport */
>>> newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>
>>> /* Set the local and remote addresses in the transport */
>>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>> + len = svc_addr_len(sa);
>>> + if (len < 0) {
>>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>>> + return;
>>
>> we're probably leaking something here.
>>
>> I don't want to fix this until it's understood well enough to fix it
>> correctly.
>
> Tom needs respond to this, but I think it would be safe to simply BUG()
> here.
Could be.--b.
Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>
>>>
>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>
>>> /* Set the local and remote addresses in the transport */
>>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>> + len = svc_addr_len(sa);
>>> + if (len < 0) {
>>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>>> + return;
>>
>> we're probably leaking something here.
Just a bit. The svcxprt_rdma, the rdma_cm_id, a bunch of other IB state...
>>
>> I don't want to fix this until it's understood well enough to fix it
>> correctly.
>
> Tom needs respond to this, but I think it would be safe to simply
> BUG() here.
My 2c. If I understand the RDMA CM behaviour correctly, it should not be
possible at this point in the code for
newxprt->sc_cm_id->route.addr.{src,dst}_addr to be anything but a valid
sockaddr_storages holding the IPv4 or (theoretically) the IPv6 address
of the server and client. So I would be happy with a BUG_ON(len < 0).
That would also render the leakage moot.
--
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.
On Mar 26, 2009, at 3:43 AM, Greg Banks wrote:
> Chuck Lever wrote:
>> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>>
>>>>
>>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>>
>>>> /* Set the local and remote addresses in the transport */
>>>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>>> + len = svc_addr_len(sa);
>>>> + if (len < 0) {
>>>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>>>> + return;
>>>
>>> we're probably leaking something here.
> Just a bit. The svcxprt_rdma, the rdma_cm_id, a bunch of other IB
> state...
>>>
>>> I don't want to fix this until it's understood well enough to fix it
>>> correctly.
>>
>> Tom needs respond to this, but I think it would be safe to simply
>> BUG() here.
>
> My 2c. If I understand the RDMA CM behaviour correctly, it should
> not be
> possible at this point in the code for
> newxprt->sc_cm_id->route.addr.{src,dst}_addr to be anything but a
> valid
> sockaddr_storages holding the IPv4 or (theoretically) the IPv6 address
> of the server and client. So I would be happy with a BUG_ON(len < 0).
> That would also render the leakage moot.
My primary objection is the negative return value from
svc_addr_len(). Perhaps it would be safer all around if
svc_addr_len() returned zero if it didn't recognize the address family
in the passed in sockaddr.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
J. Bruce Fields wrote:
> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>> The svc_addr_len() helper function can return a negative errno value,
>> but its return type is size_t, which is unsigned.
>>
>> The RDMA transport code passes this return value directly to memset(),
>> without checking first if it's negative. This could cause memset() to
>> clobber a large piece of memory if svc_addr_len() has returned an
>> error.
>
> I'd still like to understand when this can happen, to better understand
> how the error should be handled.
I don't think that the current code base can cause this to occur.
My recollection is that this code was added at the time we were
in-flight with the IPv6 integration and I was somewhat uncomfortable bug
checking on an unknown address type, however, this may in fact be the
right thing to do.
I think changing the return type to int is fine.
>
> Also:
>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> include/linux/sunrpc/svc_xprt.h | 3 ++-
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++++--
>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
>> index 0127dac..c2aa8cd 100644
>> --- a/include/linux/sunrpc/svc_xprt.h
>> +++ b/include/linux/sunrpc/svc_xprt.h
>> @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
>> return ret;
>> }
>>
>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>> +static inline int svc_addr_len(const struct sockaddr *sa)
>> {
>> switch (sa->sa_family) {
>> case AF_INET:
>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
>> case AF_INET6:
>> return sizeof(struct sockaddr_in6);
>> }
>> +
>> return -EAFNOSUPPORT;
>> }
>>
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 3d810e7..d1ec6f9 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>> struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>> struct svcxprt_rdma *newxprt;
>> struct sockaddr *sa;
>> + int len;
>>
>> /* Create a new transport */
>> newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>>
>> /* Set the local and remote addresses in the transport */
>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> + len = svc_addr_len(sa);
>> + if (len < 0) {
>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>> + return;
>
> we're probably leaking something here.
>
> I don't want to fix this until it's understood well enough to fix it
> correctly.
>
> --b.
>
>> + }
>> + svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
>> +
>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
>> - svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> + len = svc_addr_len(sa);
>> + if (len < 0) {
>> + dprintk("svcrdma: src_addr has a bad address family\n");
>> + return;
>> + }
>> + svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>>
>> /*
>> * Enqueue the new transport on the accept queue of the listening
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html