| Message ID | 20170731065016.2947796-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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)? -- 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 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 -- 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 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.-- 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 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 -- 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 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.-- 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 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 -- 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 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* -- 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 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
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 -- 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 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
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. -- 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 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 -- 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 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
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.
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;
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(-)