| Message ID | 20211025183728.181399-1-quic_qiancai@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kees Cook |
| Headers | show |
| Series | fortify: Avoid shadowing previous locals | expand |
On Mon, Oct 25, 2021 at 02:37:28PM -0400, Qian Cai wrote: > __compiletime_strlen macro expansion will shadow p_size and p_len local > variables. Just rename those in __compiletime_strlen. They don't escape their local context, though, right? i.e. I don't see a problem with the existing macro. Did you encounter a specific issue that this patch fixes? -Kees > > Signed-off-by: Qian Cai <quic_qiancai@quicinc.com> > --- > include/linux/fortify-string.h | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index fdb0a74c9ca2..155c622e4f24 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -10,18 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj > void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)"); > void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)"); > > -#define __compiletime_strlen(p) \ > -({ \ > - unsigned char *__p = (unsigned char *)(p); \ > - size_t ret = (size_t)-1; \ > - size_t p_size = __builtin_object_size(p, 1); \ > - if (p_size != (size_t)-1) { \ > - size_t p_len = p_size - 1; \ > - if (__builtin_constant_p(__p[p_len]) && \ > - __p[p_len] == '\0') \ > - ret = __builtin_strlen(__p); \ > - } \ > - ret; \ > +#define __compiletime_strlen(ptr) \ > +({ \ > + unsigned char *__ptr = (unsigned char *)(ptr); \ > + size_t ret = (size_t)-1; \ > + size_t ptr_size = __builtin_object_size(ptr, 1); \ > + if (ptr_size != (size_t)-1) { \ > + size_t ptr_len = ptr_size - 1; \ > + if (__builtin_constant_p(__ptr[ptr_len]) && \ > + __ptr[ptr_len] == '\0') \ > + ret = __builtin_strlen(__ptr); \ > + } \ > + ret; \ > }) > > #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) > -- > 2.30.2 >
On 10/25/21 3:34 PM, Kees Cook wrote: > On Mon, Oct 25, 2021 at 02:37:28PM -0400, Qian Cai wrote: >> __compiletime_strlen macro expansion will shadow p_size and p_len local >> variables. Just rename those in __compiletime_strlen. > > They don't escape their local context, though, right? i.e. I don't see a > problem with the existing macro. Did you encounter a specific issue that > this patch fixes? Yes, this is pretty minor. There are also some extra compiling warnings (W=2) from it. ./include/linux/fortify-string.h: In function 'strnlen': ./include/linux/fortify-string.h:17:9: warning: declaration of 'p_size' shadows a previous local [-Wshadow] 17 | size_t p_size = __builtin_object_size(p, 1); \ | ^~~~~~ ./include/linux/fortify-string.h:77:17: note: in expansion of macro '__compiletime_strlen' 77 | size_t p_len = __compiletime_strlen(p); | ^~~~~~~~~~~~~~~~~~~~ ./include/linux/fortify-string.h:76:9: note: shadowed declaration is here 76 | size_t p_size = __builtin_object_size(p, 1); | ^~~~~~ ./include/linux/fortify-string.h:19:10: warning: declaration of 'p_len' shadows a previous local [-Wshadow] 19 | size_t p_len = p_size - 1; \ | ^~~~~ ./include/linux/fortify-string.h:77:17: note: in expansion of macro '__compiletime_strlen' 77 | size_t p_len = __compiletime_strlen(p); | ^~~~~~~~~~~~~~~~~~~~ ./include/linux/fortify-string.h:77:9: note: shadowed declaration is here 77 | size_t p_len = __compiletime_strlen(p); | ^~~~~
On Mon, Oct 25, 2021 at 04:15:28PM -0400, Qian Cai wrote: > > > On 10/25/21 3:34 PM, Kees Cook wrote: > > On Mon, Oct 25, 2021 at 02:37:28PM -0400, Qian Cai wrote: > >> __compiletime_strlen macro expansion will shadow p_size and p_len local > >> variables. Just rename those in __compiletime_strlen. > > > > They don't escape their local context, though, right? i.e. I don't see a > > problem with the existing macro. Did you encounter a specific issue that > > this patch fixes? > > Yes, this is pretty minor. There are also some extra compiling warnings (W=2) > from it. > > ./include/linux/fortify-string.h: In function 'strnlen': > > ./include/linux/fortify-string.h:17:9: warning: declaration of 'p_size' shadows a previous local [-Wshadow] > > 17 | size_t p_size = __builtin_object_size(p, 1); \ > > | ^~~~~~ > > ./include/linux/fortify-string.h:77:17: note: in expansion of macro '__compiletime_strlen' > 77 | size_t p_len = __compiletime_strlen(p); > > | ^~~~~~~~~~~~~~~~~~~~ > > ./include/linux/fortify-string.h:76:9: note: shadowed declaration is here > > 76 | size_t p_size = __builtin_object_size(p, 1); > > | ^~~~~~ Gotcha. Yeah, we have -Wshadow=local tracked here: https://github.com/KSPP/linux/issues/152 The changes needed to fix __wait_event() are extensive, though. But yes, there's no good reason for this macro to make things worse for W=2. ;) I'd like to keep the existing names, so many just prefixing them with "__" and send a v2? Thanks!
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index fdb0a74c9ca2..155c622e4f24 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -10,18 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)"); void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)"); -#define __compiletime_strlen(p) \ -({ \ - unsigned char *__p = (unsigned char *)(p); \ - size_t ret = (size_t)-1; \ - size_t p_size = __builtin_object_size(p, 1); \ - if (p_size != (size_t)-1) { \ - size_t p_len = p_size - 1; \ - if (__builtin_constant_p(__p[p_len]) && \ - __p[p_len] == '\0') \ - ret = __builtin_strlen(__p); \ - } \ - ret; \ +#define __compiletime_strlen(ptr) \ +({ \ + unsigned char *__ptr = (unsigned char *)(ptr); \ + size_t ret = (size_t)-1; \ + size_t ptr_size = __builtin_object_size(ptr, 1); \ + if (ptr_size != (size_t)-1) { \ + size_t ptr_len = ptr_size - 1; \ + if (__builtin_constant_p(__ptr[ptr_len]) && \ + __ptr[ptr_len] == '\0') \ + ret = __builtin_strlen(__ptr); \ + } \ + ret; \ }) #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
__compiletime_strlen macro expansion will shadow p_size and p_len local variables. Just rename those in __compiletime_strlen. Signed-off-by: Qian Cai <quic_qiancai@quicinc.com> --- include/linux/fortify-string.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)