[PATCH] infiniband: avoid overflow warning

[PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

A sockaddr_in structure on the stack getting passed into rdma_ip2gid
triggers this warning, since we memcpy into a larger sockaddr_in6
structure:

In function 'memcpy',
    inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
    inlined from 'addr_event.isra.4.constprop' at drivers/infiniband/core/roce_gid_mgmt.c:693:2,
    inlined from 'inetaddr_event' at drivers/infiniband/core/roce_gid_mgmt.c:716:9:
include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter

The warning seems appropriate here, but the code is also clearly
correct, so we really just want to shut up this instance of the
output.

The best way I found so far is to avoid the memcpy() call and instead
replace it with a struct assignment.

Fixes: 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc: Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 include/rdma/ib_addr.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index 7aca12188ef3..ec5008cf5d51 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
 				       (struct in6_addr *)gid);
 		break;
 	case AF_INET6:
-		memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
+		*(struct in6_addr *)&gid->raw =
+			((struct sockaddr_in6 *)addr)->sin6_addr;
 		break;
 	default:
 		return -EINVAL;
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

A sockaddr_in structure on the stack getting passed into rdma_ip2gid
triggers this warning, since we memcpy into a larger sockaddr_in6
structure:

In function 'memcpy',
    inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
    inlined from 'addr_event.isra.4.constprop' at drivers/infiniband/core/roce_gid_mgmt.c:693:2,
    inlined from 'inetaddr_event' at drivers/infiniband/core/roce_gid_mgmt.c:716:9:
include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter

The warning seems appropriate here, but the code is also clearly
correct, so we really just want to shut up this instance of the
output.

The best way I found so far is to avoid the memcpy() call and instead
replace it with a struct assignment.

Fixes: 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/rdma/ib_addr.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index 7aca12188ef3..ec5008cf5d51 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
 				       (struct in6_addr *)gid);
 		break;
 	case AF_INET6:
-		memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
+		*(struct in6_addr *)&gid->raw =
+			((struct sockaddr_in6 *)addr)->sin6_addr;
 		break;
 	default:
 		return -EINVAL;
-- 
2.9.0

Re: [PATCH] infiniband: avoid overflow warning

[Moni Shoua]

On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> A sockaddr_in structure on the stack getting passed into rdma_ip2gid
> triggers this warning, since we memcpy into a larger sockaddr_in6
> structure:
>
> In function 'memcpy',
>     inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
>     inlined from 'addr_event.isra.4.constprop' at drivers/infiniband/core/roce_gid_mgmt.c:693:2,
>     inlined from 'inetaddr_event' at drivers/infiniband/core/roce_gid_mgmt.c:716:9:
> include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>
> The warning seems appropriate here, but the code is also clearly
> correct, so we really just want to shut up this instance of the
> output.
>
> The best way I found so far is to avoid the memcpy() call and instead
> replace it with a struct assignment.
>
> Fixes: 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
> Cc: Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> ---
>  include/rdma/ib_addr.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
> index 7aca12188ef3..ec5008cf5d51 100644
> --- a/include/rdma/ib_addr.h
> +++ b/include/rdma/ib_addr.h
> @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>                                        (struct in6_addr *)gid);
>                 break;
>         case AF_INET6:
> -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
> +               *(struct in6_addr *)&gid->raw =
> +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>                 break;
>         default:
>                 return -EINVAL;
what happens if you replace 16 with sizeof(struct in6_addr)?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Moni Shoua]

On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> A sockaddr_in structure on the stack getting passed into rdma_ip2gid
> triggers this warning, since we memcpy into a larger sockaddr_in6
> structure:
>
> In function 'memcpy',
>     inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
>     inlined from 'addr_event.isra.4.constprop' at drivers/infiniband/core/roce_gid_mgmt.c:693:2,
>     inlined from 'inetaddr_event' at drivers/infiniband/core/roce_gid_mgmt.c:716:9:
> include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>
> The warning seems appropriate here, but the code is also clearly
> correct, so we really just want to shut up this instance of the
> output.
>
> The best way I found so far is to avoid the memcpy() call and instead
> replace it with a struct assignment.
>
> Fixes: 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions")
> Cc: Daniel Micay <danielmicay@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/rdma/ib_addr.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
> index 7aca12188ef3..ec5008cf5d51 100644
> --- a/include/rdma/ib_addr.h
> +++ b/include/rdma/ib_addr.h
> @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>                                        (struct in6_addr *)gid);
>                 break;
>         case AF_INET6:
> -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
> +               *(struct in6_addr *)&gid->raw =
> +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>                 break;
>         default:
>                 return -EINVAL;
what happens if you replace 16 with sizeof(struct in6_addr)?

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> --- a/include/rdma/ib_addr.h
>> +++ b/include/rdma/ib_addr.h
>> @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>>                                        (struct in6_addr *)gid);
>>                 break;
>>         case AF_INET6:
>> -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
>> +               *(struct in6_addr *)&gid->raw =
>> +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>>                 break;
>>         default:
>>                 return -EINVAL;
> what happens if you replace 16 with sizeof(struct in6_addr)?

Same thing: the problem is that gcc already knows the size of the structure we
pass in here, and it is in fact shorter.

I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
without success. Other approaches that do work are:

- mark addr_event() as "noinline" to prevent gcc from seeing the true
size of the
  inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.

- change inetaddr_event to put a larger structure on the stack, using
  sockaddr_storage or sockaddr_in6. This would be less efficient.

- define a union of sockaddr_in and sockaddr_in6, and use that as the argument
  to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
  This is probably the cleanest approach as it gets rid of a lot of questionable
  type casts, but it's a relatively large patch and also slightly less
efficient as we have
  to zero more stack storage in some cases.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
> On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> --- a/include/rdma/ib_addr.h
>> +++ b/include/rdma/ib_addr.h
>> @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>>                                        (struct in6_addr *)gid);
>>                 break;
>>         case AF_INET6:
>> -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
>> +               *(struct in6_addr *)&gid->raw =
>> +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>>                 break;
>>         default:
>>                 return -EINVAL;
> what happens if you replace 16 with sizeof(struct in6_addr)?

Same thing: the problem is that gcc already knows the size of the structure we
pass in here, and it is in fact shorter.

I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
without success. Other approaches that do work are:

- mark addr_event() as "noinline" to prevent gcc from seeing the true
size of the
  inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.

- change inetaddr_event to put a larger structure on the stack, using
  sockaddr_storage or sockaddr_in6. This would be less efficient.

- define a union of sockaddr_in and sockaddr_in6, and use that as the argument
  to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
  This is probably the cleanest approach as it gets rid of a lot of questionable
  type casts, but it's a relatively large patch and also slightly less
efficient as we have
  to zero more stack storage in some cases.

        Arnd

Re: [PATCH] infiniband: avoid overflow warning

[Bart Van Assche]

On Mon, 2017-07-31 at 09:30 +0200, Arnd Bergmann wrote:
> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> > On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > > --- a/include/rdma/ib_addr.h
> > > +++ b/include/rdma/ib_addr.h
> > > @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
> > >                                        (struct in6_addr *)gid);
> > >                 break;
> > >         case AF_INET6:
> > > -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
> > > +               *(struct in6_addr *)&gid->raw =
> > > +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
> > >                 break;
> > >         default:
> > >                 return -EINVAL;
> > 
> > what happens if you replace 16 with sizeof(struct in6_addr)?
> 
> Same thing: the problem is that gcc already knows the size of the structure we
> pass in here, and it is in fact shorter.
> 
> I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
> without success. Other approaches that do work are:
> 
> - mark addr_event() as "noinline" to prevent gcc from seeing the true
> size of the
>   inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.
> 
> - change inetaddr_event to put a larger structure on the stack, using
>   sockaddr_storage or sockaddr_in6. This would be less efficient.
> 
> - define a union of sockaddr_in and sockaddr_in6, and use that as the argument
>   to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
>   This is probably the cleanest approach as it gets rid of a lot of questionable
>   type casts, but it's a relatively large patch and also slightly less
> efficient as we have
>   to zero more stack storage in some cases.

Hello Arnd,

So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
that is only executed if .sin_family == AF_INET6? Since this warning is the
result of incorrect interprocedural analysis by gcc, shouldn't this be
reported as a bug to the gcc authors?

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Bart Van Assche]

On Mon, 2017-07-31 at 09:30 +0200, Arnd Bergmann wrote:
> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
> > On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > --- a/include/rdma/ib_addr.h
> > > +++ b/include/rdma/ib_addr.h
> > > @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
> > >                                        (struct in6_addr *)gid);
> > >                 break;
> > >         case AF_INET6:
> > > -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
> > > +               *(struct in6_addr *)&gid->raw =
> > > +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
> > >                 break;
> > >         default:
> > >                 return -EINVAL;
> > 
> > what happens if you replace 16 with sizeof(struct in6_addr)?
> 
> Same thing: the problem is that gcc already knows the size of the structure we
> pass in here, and it is in fact shorter.
> 
> I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
> without success. Other approaches that do work are:
> 
> - mark addr_event() as "noinline" to prevent gcc from seeing the true
> size of the
>   inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.
> 
> - change inetaddr_event to put a larger structure on the stack, using
>   sockaddr_storage or sockaddr_in6. This would be less efficient.
> 
> - define a union of sockaddr_in and sockaddr_in6, and use that as the argument
>   to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
>   This is probably the cleanest approach as it gets rid of a lot of questionable
>   type casts, but it's a relatively large patch and also slightly less
> efficient as we have
>   to zero more stack storage in some cases.

Hello Arnd,

So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
that is only executed if .sin_family == AF_INET6? Since this warning is the
result of incorrect interprocedural analysis by gcc, shouldn't this be
reported as a bug to the gcc authors?

Thanks,

Bart.

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org> wrote:
> On Mon, 2017-07-31 at 09:30 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> > On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > > --- a/include/rdma/ib_addr.h
>> > > +++ b/include/rdma/ib_addr.h
>> > > @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>> > >                                        (struct in6_addr *)gid);
>> > >                 break;
>> > >         case AF_INET6:
>> > > -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
>> > > +               *(struct in6_addr *)&gid->raw =
>> > > +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>> > >                 break;
>> > >         default:
>> > >                 return -EINVAL;
>> >
>> > what happens if you replace 16 with sizeof(struct in6_addr)?
>>
>> Same thing: the problem is that gcc already knows the size of the structure we
>> pass in here, and it is in fact shorter.
>>
>> I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
>> without success. Other approaches that do work are:
>>
>> - mark addr_event() as "noinline" to prevent gcc from seeing the true
>> size of the
>>   inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.
>>
>> - change inetaddr_event to put a larger structure on the stack, using
>>   sockaddr_storage or sockaddr_in6. This would be less efficient.
>>
>> - define a union of sockaddr_in and sockaddr_in6, and use that as the argument
>>   to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
>>   This is probably the cleanest approach as it gets rid of a lot of questionable
>>   type casts, but it's a relatively large patch and also slightly less
>> efficient as we have
>>   to zero more stack storage in some cases.
>
>
> So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
> that is only executed if .sin_family == AF_INET6? Since this warning is the
> result of incorrect interprocedural analysis by gcc, shouldn't this be
> reported as a bug to the gcc authors?

I think the interprocedural analysis here is just a little worse than it could
be, but it's not actually correct.  It's not gcc that prints the warning (if
it did, then I'd agree it would be a gcc bug) but the warning is triggered
intentionally by the fortified version of memcpy in include/linux/string.h.

The problem as I understand it is that gcc cannot guarantee that it
tracks the value of addr->sa_family at  least as far as the size of the
stack object, and it has no strict reason to do so, so the inlined
rdma_ip2gid() will still contain both cases.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Mon, 2017-07-31 at 09:30 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>> > On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > > --- a/include/rdma/ib_addr.h
>> > > +++ b/include/rdma/ib_addr.h
>> > > @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>> > >                                        (struct in6_addr *)gid);
>> > >                 break;
>> > >         case AF_INET6:
>> > > -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
>> > > +               *(struct in6_addr *)&gid->raw =
>> > > +                       ((struct sockaddr_in6 *)addr)->sin6_addr;
>> > >                 break;
>> > >         default:
>> > >                 return -EINVAL;
>> >
>> > what happens if you replace 16 with sizeof(struct in6_addr)?
>>
>> Same thing: the problem is that gcc already knows the size of the structure we
>> pass in here, and it is in fact shorter.
>>
>> I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer,
>> without success. Other approaches that do work are:
>>
>> - mark addr_event() as "noinline" to prevent gcc from seeing the true
>> size of the
>>   inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly.
>>
>> - change inetaddr_event to put a larger structure on the stack, using
>>   sockaddr_storage or sockaddr_in6. This would be less efficient.
>>
>> - define a union of sockaddr_in and sockaddr_in6, and use that as the argument
>>   to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type.
>>   This is probably the cleanest approach as it gets rid of a lot of questionable
>>   type casts, but it's a relatively large patch and also slightly less
>> efficient as we have
>>   to zero more stack storage in some cases.
>
>
> So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
> that is only executed if .sin_family == AF_INET6? Since this warning is the
> result of incorrect interprocedural analysis by gcc, shouldn't this be
> reported as a bug to the gcc authors?

I think the interprocedural analysis here is just a little worse than it could
be, but it's not actually correct.  It's not gcc that prints the warning (if
it did, then I'd agree it would be a gcc bug) but the warning is triggered
intentionally by the fortified version of memcpy in include/linux/string.h.

The problem as I understand it is that gcc cannot guarantee that it
tracks the value of addr->sa_family at  least as far as the size of the
stack object, and it has no strict reason to do so, so the inlined
rdma_ip2gid() will still contain both cases.

      Arnd

Re: [PATCH] infiniband: avoid overflow warning

[Bart Van Assche]

On Mon, 2017-07-31 at 18:04 +0200, Arnd Bergmann wrote:
> On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org> wrote:
> > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
> > that is only executed if .sin_family == AF_INET6? Since this warning is the
> > result of incorrect interprocedural analysis by gcc, shouldn't this be
> > reported as a bug to the gcc authors?
> 
> I think the interprocedural analysis here is just a little worse than it could
> be, but it's not actually correct.  It's not gcc that prints the warning (if
> it did, then I'd agree it would be a gcc bug) but the warning is triggered
> intentionally by the fortified version of memcpy in include/linux/string.h.
> 
> The problem as I understand it is that gcc cannot guarantee that it
> tracks the value of addr->sa_family at  least as far as the size of the
> stack object, and it has no strict reason to do so, so the inlined
> rdma_ip2gid() will still contain both cases.

Hello Arnd,

Had you already considered to uninline the rdma_ip2gid() function?

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Bart Van Assche]

On Mon, 2017-07-31 at 18:04 +0200, Arnd Bergmann wrote:
> On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
> > that is only executed if .sin_family == AF_INET6? Since this warning is the
> > result of incorrect interprocedural analysis by gcc, shouldn't this be
> > reported as a bug to the gcc authors?
> 
> I think the interprocedural analysis here is just a little worse than it could
> be, but it's not actually correct.  It's not gcc that prints the warning (if
> it did, then I'd agree it would be a gcc bug) but the warning is triggered
> intentionally by the fortified version of memcpy in include/linux/string.h.
> 
> The problem as I understand it is that gcc cannot guarantee that it
> tracks the value of addr->sa_family at  least as far as the size of the
> stack object, and it has no strict reason to do so, so the inlined
> rdma_ip2gid() will still contain both cases.

Hello Arnd,

Had you already considered to uninline the rdma_ip2gid() function?

Bart.

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 6:17 PM, Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org> wrote:
> On Mon, 2017-07-31 at 18:04 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org> wrote:
>> > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
>> > that is only executed if .sin_family == AF_INET6? Since this warning is the
>> > result of incorrect interprocedural analysis by gcc, shouldn't this be
>> > reported as a bug to the gcc authors?
>>
>> I think the interprocedural analysis here is just a little worse than it could
>> be, but it's not actually correct.  It's not gcc that prints the warning (if
>> it did, then I'd agree it would be a gcc bug) but the warning is triggered
>> intentionally by the fortified version of memcpy in include/linux/string.h.
>>
>> The problem as I understand it is that gcc cannot guarantee that it
>> tracks the value of addr->sa_family at  least as far as the size of the
>> stack object, and it has no strict reason to do so, so the inlined
>> rdma_ip2gid() will still contain both cases.
>
> Hello Arnd,
>
> Had you already considered to uninline the rdma_ip2gid() function?

Not really, that would prevent the normal optimization from happening,
so that would be worse than uninlining addr_event() as I tried.

It would of course get rid of the warning, so if you think that's a better
solution, I won't complain.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 6:17 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Mon, 2017-07-31 at 18:04 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>> > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code
>> > that is only executed if .sin_family == AF_INET6? Since this warning is the
>> > result of incorrect interprocedural analysis by gcc, shouldn't this be
>> > reported as a bug to the gcc authors?
>>
>> I think the interprocedural analysis here is just a little worse than it could
>> be, but it's not actually correct.  It's not gcc that prints the warning (if
>> it did, then I'd agree it would be a gcc bug) but the warning is triggered
>> intentionally by the fortified version of memcpy in include/linux/string.h.
>>
>> The problem as I understand it is that gcc cannot guarantee that it
>> tracks the value of addr->sa_family at  least as far as the size of the
>> stack object, and it has no strict reason to do so, so the inlined
>> rdma_ip2gid() will still contain both cases.
>
> Hello Arnd,
>
> Had you already considered to uninline the rdma_ip2gid() function?

Not really, that would prevent the normal optimization from happening,
so that would be worse than uninlining addr_event() as I tried.

It would of course get rid of the warning, so if you think that's a better
solution, I won't complain.

       Arnd

Re: [PATCH] infiniband: avoid overflow warning

[Daniel Micay]

On Mon, 2017-07-31 at 21:19 +0200, Arnd Bergmann wrote:
> On Mon, Jul 31, 2017 at 6:17 PM, Bart Van Assche <Bart.VanAssche@wdc.c
> om> wrote:
> > On Mon, 2017-07-31 at 18:04 +0200, Arnd Bergmann wrote:
> > > On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche <Bart.VanAssche@w
> > > dc.com> wrote:
> > > > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns
> > > > about code
> > > > that is only executed if .sin_family == AF_INET6? Since this
> > > > warning is the
> > > > result of incorrect interprocedural analysis by gcc, shouldn't
> > > > this be
> > > > reported as a bug to the gcc authors?
> > > 
> > > I think the interprocedural analysis here is just a little worse
> > > than it could
> > > be, but it's not actually correct.  It's not gcc that prints the
> > > warning (if
> > > it did, then I'd agree it would be a gcc bug) but the warning is
> > > triggered
> > > intentionally by the fortified version of memcpy in
> > > include/linux/string.h.
> > > 
> > > The problem as I understand it is that gcc cannot guarantee that
> > > it
> > > tracks the value of addr->sa_family at  least as far as the size
> > > of the
> > > stack object, and it has no strict reason to do so, so the inlined
> > > rdma_ip2gid() will still contain both cases.
> > 
> > Hello Arnd,
> > 
> > Had you already considered to uninline the rdma_ip2gid() function?
> 
> Not really, that would prevent the normal optimization from happening,
> so that would be worse than uninlining addr_event() as I tried.
> 
> It would of course get rid of the warning, so if you think that's a
> better
> solution, I won't complain.
> 
>        Arnd

You could also use __memcpy but using a struct assignment seems cleaner.

The compile-time fortify errors aren't perfect since they rely on GCC
being able to optimize them out and there can be dead code that has
intentional overflows, etc. Their purpose is just to move many runtime
errors (which don't have these false positives) to compile-time since
it's a lot less painful to deal with a few false positives like this
than errors slipping through to runtime (although it's less important
since it's going to be using WARN for the time being).

If the compile-time errors would removed, all of the real overflows
would still be detected at runtime. One option would be having something
like #define __NO_FORTIFY but *only* disabling the compile-time part of
the checks to work around false positives. *shrug*

Re: [PATCH] infiniband: avoid overflow warning

[Kees Cook]

On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>> On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> --- a/include/rdma/ib_addr.h
>>> +++ b/include/rdma/ib_addr.h
>>> @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>>>                                        (struct in6_addr *)gid);
>>>                 break;
>>>         case AF_INET6:
>>> -               memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16);
>>> +               *(struct in6_addr *)&gid->raw =
>>> +                       ((struct sockaddr_in6 *)addr)->sin6_addr;

This seems reasonable.

>>>                 break;
>>>         default:
>>>                 return -EINVAL;
>> what happens if you replace 16 with sizeof(struct in6_addr)?
>
> Same thing: the problem is that gcc already knows the size of the structure we
> pass in here, and it is in fact shorter.

So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
caller's actual 128 byte struct sockaddr_storage, and looking only at
struct sockaddr? That seems really weird.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>>>                 break;
>>>>         default:
>>>>                 return -EINVAL;
>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>
>> Same thing: the problem is that gcc already knows the size of the structure we
>> pass in here, and it is in fact shorter.
>
> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
> caller's actual 128 byte struct sockaddr_storage, and looking only at
> struct sockaddr? That seems really weird.

Using a sockaddr_storage on the stack would address the warning, but
the question was about just changing the hardcoded 16 to a sizeof()
operation, and that has no effect.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>>>>                 break;
>>>>         default:
>>>>                 return -EINVAL;
>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>
>> Same thing: the problem is that gcc already knows the size of the structure we
>> pass in here, and it is in fact shorter.
>
> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
> caller's actual 128 byte struct sockaddr_storage, and looking only at
> struct sockaddr? That seems really weird.

Using a sockaddr_storage on the stack would address the warning, but
the question was about just changing the hardcoded 16 to a sizeof()
operation, and that has no effect.

        Arnd

Re: [PATCH] infiniband: avoid overflow warning

[Kees Cook]

On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>>>>                 break;
>>>>>         default:
>>>>>                 return -EINVAL;
>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>
>>> Same thing: the problem is that gcc already knows the size of the structure we
>>> pass in here, and it is in fact shorter.
>>
>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>> struct sockaddr? That seems really weird.
>
> Using a sockaddr_storage on the stack would address the warning, but
> the question was about just changing the hardcoded 16 to a sizeof()
> operation, and that has no effect.

Right, I didn't mean that; I was curious why the fortify macro
resulted in an error at all. The callers are casting from struct
sockaddr_storage (large enough) to struct sockaddr (not large enough),
and then the inline is casting back to sockaddr_in6 (large enough). I
would have expected fortify to check either sockaddr_storage or
sockaddr_in6, but not sockaddr.

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Kees Cook]

On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>>>>>                 break;
>>>>>         default:
>>>>>                 return -EINVAL;
>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>
>>> Same thing: the problem is that gcc already knows the size of the structure we
>>> pass in here, and it is in fact shorter.
>>
>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>> struct sockaddr? That seems really weird.
>
> Using a sockaddr_storage on the stack would address the warning, but
> the question was about just changing the hardcoded 16 to a sizeof()
> operation, and that has no effect.

Right, I didn't mean that; I was curious why the fortify macro
resulted in an error at all. The callers are casting from struct
sockaddr_storage (large enough) to struct sockaddr (not large enough),
and then the inline is casting back to sockaddr_in6 (large enough). I
would have expected fortify to check either sockaddr_storage or
sockaddr_in6, but not sockaddr.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] infiniband: avoid overflow warning

[Daniel Micay]

On Mon, 2017-07-31 at 14:18 -0700, Kees Cook wrote:
> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > wrote:
> > > On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > > wrote:
> > > > On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > > wrote:
> > > > > >                 break;
> > > > > >         default:
> > > > > >                 return -EINVAL;
> > > > > 
> > > > > what happens if you replace 16 with sizeof(struct in6_addr)?
> > > > 
> > > > Same thing: the problem is that gcc already knows the size of
> > > > the structure we
> > > > pass in here, and it is in fact shorter.
> > > 
> > > So gcc is ignoring both the cast (to 16 byte struct in6_addr) and
> > > the
> > > caller's actual 128 byte struct sockaddr_storage, and looking only
> > > at
> > > struct sockaddr? That seems really weird.
> > 
> > Using a sockaddr_storage on the stack would address the warning, but
> > the question was about just changing the hardcoded 16 to a sizeof()
> > operation, and that has no effect.
> 
> Right, I didn't mean that; I was curious why the fortify macro
> resulted in an error at all. The callers are casting from struct
> sockaddr_storage (large enough) to struct sockaddr (not large enough),
> and then the inline is casting back to sockaddr_in6 (large enough). I
> would have expected fortify to check either sockaddr_storage or
> sockaddr_in6, but not sockaddr.
> 
> -Kees
> 

I don't think that's quite what's happening. It will report unknown if
it can't find allocation sites or other guarantees of size. There are no
alloc_size markers yet so it *mostly* does stack / global checks. It
won't infer sizes based on pointer types. It's not a heuristic.

Hoowever, fortify / -fsanitize=object-size can indirectly uncover other
forms of undefined behavior though. Code may rely on doing something
undefined that GCC actively assumes can't happen for optimization. It
can have false positives due to dead code with the compile-time errors
but if the code is actually called with the size that it rejects as
invalid, then it's unlikely there isn't a bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Daniel Micay]

On Mon, 2017-07-31 at 14:18 -0700, Kees Cook wrote:
> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@chromium.org>
> > wrote:
> > > On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de>
> > > wrote:
> > > > On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com>
> > > > wrote:
> > > > > >                 break;
> > > > > >         default:
> > > > > >                 return -EINVAL;
> > > > > 
> > > > > what happens if you replace 16 with sizeof(struct in6_addr)?
> > > > 
> > > > Same thing: the problem is that gcc already knows the size of
> > > > the structure we
> > > > pass in here, and it is in fact shorter.
> > > 
> > > So gcc is ignoring both the cast (to 16 byte struct in6_addr) and
> > > the
> > > caller's actual 128 byte struct sockaddr_storage, and looking only
> > > at
> > > struct sockaddr? That seems really weird.
> > 
> > Using a sockaddr_storage on the stack would address the warning, but
> > the question was about just changing the hardcoded 16 to a sizeof()
> > operation, and that has no effect.
> 
> Right, I didn't mean that; I was curious why the fortify macro
> resulted in an error at all. The callers are casting from struct
> sockaddr_storage (large enough) to struct sockaddr (not large enough),
> and then the inline is casting back to sockaddr_in6 (large enough). I
> would have expected fortify to check either sockaddr_storage or
> sockaddr_in6, but not sockaddr.
> 
> -Kees
> 

I don't think that's quite what's happening. It will report unknown if
it can't find allocation sites or other guarantees of size. There are no
alloc_size markers yet so it *mostly* does stack / global checks. It
won't infer sizes based on pointer types. It's not a heuristic.

Hoowever, fortify / -fsanitize=object-size can indirectly uncover other
forms of undefined behavior though. Code may rely on doing something
undefined that GCC actively assumes can't happen for optimization. It
can have false positives due to dead code with the compile-time errors
but if the code is actually called with the size that it rejects as
invalid, then it's unlikely there isn't a bug.

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 11:18 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>>>>>                 break;
>>>>>>         default:
>>>>>>                 return -EINVAL;
>>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>>
>>>> Same thing: the problem is that gcc already knows the size of the structure we
>>>> pass in here, and it is in fact shorter.
>>>
>>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>>> struct sockaddr? That seems really weird.
>>
>> Using a sockaddr_storage on the stack would address the warning, but
>> the question was about just changing the hardcoded 16 to a sizeof()
>> operation, and that has no effect.
>
> Right, I didn't mean that; I was curious why the fortify macro
> resulted in an error at all. The callers are casting from struct
> sockaddr_storage (large enough) to struct sockaddr (not large enough),
> and then the inline is casting back to sockaddr_in6 (large enough). I
> would have expected fortify to check either sockaddr_storage or
> sockaddr_in6, but not sockaddr.

To clarify: this happens in inetaddr_event(), which has a sockaddr_in
on the stack, not a sockaddr_storage. I tried casting the sockaddr_in
pointer to sockaddr_storage, but that did not help. Changing the
type of the stack variable to sockaddr_storage does help.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 11:18 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>>>>>>                 break;
>>>>>>         default:
>>>>>>                 return -EINVAL;
>>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>>
>>>> Same thing: the problem is that gcc already knows the size of the structure we
>>>> pass in here, and it is in fact shorter.
>>>
>>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>>> struct sockaddr? That seems really weird.
>>
>> Using a sockaddr_storage on the stack would address the warning, but
>> the question was about just changing the hardcoded 16 to a sizeof()
>> operation, and that has no effect.
>
> Right, I didn't mean that; I was curious why the fortify macro
> resulted in an error at all. The callers are casting from struct
> sockaddr_storage (large enough) to struct sockaddr (not large enough),
> and then the inline is casting back to sockaddr_in6 (large enough). I
> would have expected fortify to check either sockaddr_storage or
> sockaddr_in6, but not sockaddr.

To clarify: this happens in inetaddr_event(), which has a sockaddr_in
on the stack, not a sockaddr_storage. I tried casting the sockaddr_in
pointer to sockaddr_storage, but that did not help. Changing the
type of the stack variable to sockaddr_storage does help.

        Arnd

Re: [PATCH] infiniband: avoid overflow warning

[Kees Cook]

On Mon, Jul 31, 2017 at 2:52 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Mon, Jul 31, 2017 at 11:18 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>>>>>>                 break;
>>>>>>>         default:
>>>>>>>                 return -EINVAL;
>>>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>>>
>>>>> Same thing: the problem is that gcc already knows the size of the structure we
>>>>> pass in here, and it is in fact shorter.
>>>>
>>>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>>>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>>>> struct sockaddr? That seems really weird.
>>>
>>> Using a sockaddr_storage on the stack would address the warning, but
>>> the question was about just changing the hardcoded 16 to a sizeof()
>>> operation, and that has no effect.
>>
>> Right, I didn't mean that; I was curious why the fortify macro
>> resulted in an error at all. The callers are casting from struct
>> sockaddr_storage (large enough) to struct sockaddr (not large enough),
>> and then the inline is casting back to sockaddr_in6 (large enough). I
>> would have expected fortify to check either sockaddr_storage or
>> sockaddr_in6, but not sockaddr.
>
> To clarify: this happens in inetaddr_event(), which has a sockaddr_in
> on the stack, not a sockaddr_storage. I tried casting the sockaddr_in
> pointer to sockaddr_storage, but that did not help. Changing the

Oooh, I see now. Yeah, addr_event() sees it directly as struct
sockaddr and even with the resulting inlining into inetaddr_event(),
the dead-code analysis doesn't eliminate the AF_INET6 case, which is a
shame.

> type of the stack variable to sockaddr_storage does help.

That seems like an unfortunate waste of stack space for a false
positive. :) I think your original fix is fine. (In fact, I think it's
actually more robust since there isn't a hard-coded "16" -- not that
it'll ever change, of course.)

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Kees Cook]

On Mon, Jul 31, 2017 at 2:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 31, 2017 at 11:18 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@mellanox.com> wrote:
>>>>>>>                 break;
>>>>>>>         default:
>>>>>>>                 return -EINVAL;
>>>>>> what happens if you replace 16 with sizeof(struct in6_addr)?
>>>>>
>>>>> Same thing: the problem is that gcc already knows the size of the structure we
>>>>> pass in here, and it is in fact shorter.
>>>>
>>>> So gcc is ignoring both the cast (to 16 byte struct in6_addr) and the
>>>> caller's actual 128 byte struct sockaddr_storage, and looking only at
>>>> struct sockaddr? That seems really weird.
>>>
>>> Using a sockaddr_storage on the stack would address the warning, but
>>> the question was about just changing the hardcoded 16 to a sizeof()
>>> operation, and that has no effect.
>>
>> Right, I didn't mean that; I was curious why the fortify macro
>> resulted in an error at all. The callers are casting from struct
>> sockaddr_storage (large enough) to struct sockaddr (not large enough),
>> and then the inline is casting back to sockaddr_in6 (large enough). I
>> would have expected fortify to check either sockaddr_storage or
>> sockaddr_in6, but not sockaddr.
>
> To clarify: this happens in inetaddr_event(), which has a sockaddr_in
> on the stack, not a sockaddr_storage. I tried casting the sockaddr_in
> pointer to sockaddr_storage, but that did not help. Changing the

Oooh, I see now. Yeah, addr_event() sees it directly as struct
sockaddr and even with the resulting inlining into inetaddr_event(),
the dead-code analysis doesn't eliminate the AF_INET6 case, which is a
shame.

> type of the stack variable to sockaddr_storage does help.

That seems like an unfortunate waste of stack space for a false
positive. :) I think your original fix is fine. (In fact, I think it's
actually more robust since there isn't a hard-coded "16" -- not that
it'll ever change, of course.)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] infiniband: avoid overflow warning

[Doug Ledford]

On Mon, 2017-07-31 at 08:50 +0200, Arnd Bergmann wrote:
> A sockaddr_in structure on the stack getting passed into rdma_ip2gid
> triggers this warning, since we memcpy into a larger sockaddr_in6
> structure:
> 
> In function 'memcpy',
>     inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
>     inlined from 'addr_event.isra.4.constprop' at
> drivers/infiniband/core/roce_gid_mgmt.c:693:2,
>     inlined from 'inetaddr_event' at
> drivers/infiniband/core/roce_gid_mgmt.c:716:9:
> include/linux/string.h:305:4: error: call to '__read_overflow2'
> declared with attribute error: detected read beyond size of object
> passed as 2nd parameter
> 
> The warning seems appropriate here, but the code is also clearly
> correct, so we really just want to shut up this instance of the
> output.
> 
> The best way I found so far is to avoid the memcpy() call and instead
> replace it with a struct assignment.
> 
> Fixes: 6974f0c4555e ("include/linux/string.h: add the option of
> fortified string.h functions")
> Cc: Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Thanks, applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: avoid overflow warning

[Doug Ledford]

On Mon, 2017-07-31 at 08:50 +0200, Arnd Bergmann wrote:
> A sockaddr_in structure on the stack getting passed into rdma_ip2gid
> triggers this warning, since we memcpy into a larger sockaddr_in6
> structure:
> 
> In function 'memcpy',
>     inlined from 'rdma_ip2gid' at include/rdma/ib_addr.h:175:3,
>     inlined from 'addr_event.isra.4.constprop' at
> drivers/infiniband/core/roce_gid_mgmt.c:693:2,
>     inlined from 'inetaddr_event' at
> drivers/infiniband/core/roce_gid_mgmt.c:716:9:
> include/linux/string.h:305:4: error: call to '__read_overflow2'
> declared with attribute error: detected read beyond size of object
> passed as 2nd parameter
> 
> The warning seems appropriate here, but the code is also clearly
> correct, so we really just want to shut up this instance of the
> output.
> 
> The best way I found so far is to avoid the memcpy() call and instead
> replace it with a struct assignment.
> 
> Fixes: 6974f0c4555e ("include/linux/string.h: add the option of
> fortified string.h functions")
> Cc: Daniel Micay <danielmicay@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks, applied.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD