Messages in this thread | | | Subject | Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen | From | Joseph Qi <> | Date | Wed, 29 Sep 2021 17:12:50 +0800 |
| |
On 9/29/21 2:24 PM, Valentin Vidić wrote: > On Wed, Sep 29, 2021 at 10:38:59AM +0800, Joseph Qi wrote: >> Okay, you are right, strlen(src) is indeed wrong here. >> >> But please note that in strlcpy(): >> size_t ret = strlen(src); >> if (size) { >> size_t len = (ret >= size) ? size - 1 : ret; >> memcpy(dest, src, len); >> dest[len] = '\0'; >> } >> >> Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the >> coming byte is not null, say it is 10. >> The input size is 5, so len will finally be 4. >> So dest is still correct ending with null byte. No overflow happens. >> So the problem here is the wrong return value, but it is discarded in >> ocfs2_initialize_super(). > > strlcpy starts with a call to strlen(src) and this is where the read overflow > happens. If the kernel is compiled with CONFIG_FORTIFY_SOURCE this gets > executed instead (include/linux/fortify-string.h): > > __FORTIFY_INLINE __kernel_size_t strlen(const char *p) > { > __kernel_size_t ret; > size_t p_size = __builtin_object_size(p, 1); > > /* Work around gcc excess stack consumption issue */ > if (p_size == (size_t)-1 || > (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0')) > return __underlying_strlen(p); > ret = strnlen(p, p_size); > if (p_size <= ret) > fortify_panic(__func__); > return ret; > } > > So while strlcpy did work before this fortify check, it is probably not the > best option anymore due to the missing null terminator in the source. > Got it, it really triggers panic in strlen(). So could you please update the commit log? I think CONFIG_FORTIFY_SOURCE is necessary information since it is not default enabled. And add comments with your changes, e.g.
/* * ci_stack and ci_cluster in ocfs2_cluster_info may not null * terminated, make sure no overflow happens here. */
BTW, since we use kzalloc to alloc osb, so we don't have to manually set the last null byte.
Thanks, Joseph
| |