| Message ID | 20190311153857.563743-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | a623a7a1a5670c25a16881f5078072d272d96b71 |
| Headers | show |
| Series | y2038: fix socket.h header inclusion | expand |
From: Arnd Bergmann <arnd@arndb.de> Date: Mon, 11 Mar 2019 16:38:17 +0100 > Referencing the __kernel_long_t type caused some user space applications > to stop compiling when they had not already included linux/posix_types.h, > e.g. > > s/multicast.c -o ext/sockets/multicast.lo > In file included from /builddir/build/BUILD/php-7.3.3/main/php.h:468, > from /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c:27: > /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c: In function 'zm_startup_sockets': > /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c:776:40: error: '__kernel_long_t' undeclared (first use in this function) > 776 | REGISTER_LONG_CONSTANT("SO_SNDTIMEO", SO_SNDTIMEO, CONST_CS | CONST_PERSISTENT); > > It is safe to include that header here, since it only contains kernel > internal types that do not conflict with other user space types. > > It's still possible that some related build failures remain, but those > are likely to be for code that is not already y2038 safe. > > Reported-by: Laura Abbott <labbott@redhat.com> > Fixes: a9beb86ae6e5 ("sock: Add SO_RCVTIMEO_NEW and SO_SNDTIMEO_NEW") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Applied, thanks.
* Arnd Bergmann: > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > index 0d0fddb7e738..976e89b116e5 100644 > --- a/arch/alpha/include/uapi/asm/socket.h > +++ b/arch/alpha/include/uapi/asm/socket.h > @@ -2,8 +2,8 @@ > #ifndef _UAPI_ASM_SOCKET_H > #define _UAPI_ASM_SOCKET_H > > +#include <linux/posix_types.h> > #include <asm/sockios.h> > -#include <asm/bitsperlong.h> This breaks POSIX conformance in glibc because the <linux/posix_types.h> header is not namespace clean. It contains the identifiers fds_bits and val: unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))]; int val[2]; We could duplicate some of the SO_* constants for POSIX mode in glibc, but it would be nice to avoid that. Is there a different way of fixing this on the kernel side that avoids including <linux/posix_types.h>?
On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Arnd Bergmann: > > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > > index 0d0fddb7e738..976e89b116e5 100644 > > --- a/arch/alpha/include/uapi/asm/socket.h > > +++ b/arch/alpha/include/uapi/asm/socket.h > > @@ -2,8 +2,8 @@ > > #ifndef _UAPI_ASM_SOCKET_H > > #define _UAPI_ASM_SOCKET_H > > > > +#include <linux/posix_types.h> > > #include <asm/sockios.h> > > -#include <asm/bitsperlong.h> > > This breaks POSIX conformance in glibc because the > <linux/posix_types.h> header is not namespace clean. It contains the > identifiers fds_bits and val: > > unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))]; > > int val[2]; What is problematic about the struct members here? I had thought that only the struct names have to be in a namespace to be usable here, but not the members. The only part that might be problematic is #undef __FD_SETSIZE #define __FD_SETSIZE 1024 but we already get that from a number of other inclusions of linux/posix_types.h. Is this what you mean? > We could duplicate some of the SO_* constants for POSIX mode in glibc, > but it would be nice to avoid that. > > Is there a different way of fixing this on the kernel side that avoids > including <linux/posix_types.h>? We could use asm/posix_types.h instead of linux/posix_types.h, would that address your concern? Arnd
* Arnd Bergmann: > On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote: >> >> * Arnd Bergmann: >> >> > diff --git a/arch/alpha/include/uapi/asm/socket.h >> > b/arch/alpha/include/uapi/asm/socket.h >> > index 0d0fddb7e738..976e89b116e5 100644 >> > --- a/arch/alpha/include/uapi/asm/socket.h >> > +++ b/arch/alpha/include/uapi/asm/socket.h >> > @@ -2,8 +2,8 @@ >> > #ifndef _UAPI_ASM_SOCKET_H >> > #define _UAPI_ASM_SOCKET_H >> > >> > +#include <linux/posix_types.h> >> > #include <asm/sockios.h> >> > -#include <asm/bitsperlong.h> >> >> This breaks POSIX conformance in glibc because the >> <linux/posix_types.h> header is not namespace clean. It contains the >> identifiers fds_bits and val: >> >> unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))]; >> >> int val[2]; > > What is problematic about the struct members here? I had thought that > only the struct names have to be in a namespace to be usable here, > but not the members. According POSIX, a user can do this: #define fds_bits 1024 before including the <sys/socket.h> header file. Similarly for val. Since glibc pulls in <asm/socket.h> indirectly, the result is a parse error, even though the programmer did nothing wrong (fds_bits is not an identifier used by POSIX, nor is it in the implementation namespace, ans <sys/socket.h> is a POSIX header). > We could use asm/posix_types.h instead of linux/posix_types.h, > would that address your concern? It should fix the fds_bits case, I think. But <asm-generic/posix_types.h> still uses val, so that part of the issue remains.
On Fri, Mar 15, 2019 at 2:20 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Arnd Bergmann: > > > On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote: > >> > >> * Arnd Bergmann: > >> > >> > diff --git a/arch/alpha/include/uapi/asm/socket.h > >> > b/arch/alpha/include/uapi/asm/socket.h > >> > index 0d0fddb7e738..976e89b116e5 100644 > >> > --- a/arch/alpha/include/uapi/asm/socket.h > >> > +++ b/arch/alpha/include/uapi/asm/socket.h > >> > @@ -2,8 +2,8 @@ > >> > #ifndef _UAPI_ASM_SOCKET_H > >> > #define _UAPI_ASM_SOCKET_H > >> > > >> > +#include <linux/posix_types.h> > >> > #include <asm/sockios.h> > >> > -#include <asm/bitsperlong.h> > >> > >> This breaks POSIX conformance in glibc because the > >> <linux/posix_types.h> header is not namespace clean. It contains the > >> identifiers fds_bits and val: > >> > >> unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))]; > >> > >> int val[2]; > > > > What is problematic about the struct members here? I had thought that > > only the struct names have to be in a namespace to be usable here, > > but not the members. > > According POSIX, a user can do this: > > #define fds_bits 1024 > > before including the <sys/socket.h> header file. Similarly for val. > > Since glibc pulls in <asm/socket.h> indirectly, the result is a parse > error, even though the programmer did nothing wrong (fds_bits is not > an identifier used by POSIX, nor is it in the implementation > namespace, ans <sys/socket.h> is a POSIX header). > > > We could use asm/posix_types.h instead of linux/posix_types.h, > > would that address your concern? > > It should fix the fds_bits case, I think. But > <asm-generic/posix_types.h> still uses val, so that part of the issue > remains. Would moving kernel namespace types(__kernel prefix) to a different header file(kernel_types.h?) and then including this from linux/posix_types.h. And, for socket.h just including kernel_types.h make sense? -Deepa
On Sun, Mar 17, 2019 at 7:20 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > On Fri, Mar 15, 2019 at 2:20 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > > On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > >> > diff --git a/arch/alpha/include/uapi/asm/socket.h > > >> > b/arch/alpha/include/uapi/asm/socket.h > > >> > index 0d0fddb7e738..976e89b116e5 100644 > > >> > --- a/arch/alpha/include/uapi/asm/socket.h > > >> > +++ b/arch/alpha/include/uapi/asm/socket.h > > >> > @@ -2,8 +2,8 @@ > > >> > #ifndef _UAPI_ASM_SOCKET_H > > >> > #define _UAPI_ASM_SOCKET_H > > >> > > > >> > +#include <linux/posix_types.h> > > >> > #include <asm/sockios.h> > > >> > -#include <asm/bitsperlong.h> > > >> > > >> This breaks POSIX conformance in glibc because the > > >> <linux/posix_types.h> header is not namespace clean. It contains the > > >> identifiers fds_bits and val: > > >> > > >> unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))]; > > >> > > >> int val[2]; > > > > > > What is problematic about the struct members here? I had thought that > > > only the struct names have to be in a namespace to be usable here, > > > but not the members. > > > > According POSIX, a user can do this: > > > > #define fds_bits 1024 > > > > before including the <sys/socket.h> header file. Similarly for val. > > > > Since glibc pulls in <asm/socket.h> indirectly, the result is a parse > > error, even though the programmer did nothing wrong (fds_bits is not > > an identifier used by POSIX, nor is it in the implementation > > namespace, ans <sys/socket.h> is a POSIX header). Ok, I see. Thanks for the explanation! > > > We could use asm/posix_types.h instead of linux/posix_types.h, > > > would that address your concern? > > > > It should fix the fds_bits case, I think. But > > <asm-generic/posix_types.h> still uses val, so that part of the issue > > remains. > > Would moving kernel namespace types(__kernel prefix) to a different > header file(kernel_types.h?) and then including this from > linux/posix_types.h. > And, for socket.h just including kernel_types.h make sense? I fear we have considered linux/posix_types.h to be something that can be included anywhere for a long time, so it may be better to ensure that this is actually the case, and avoid the problem with those two structures but leave the rest untouched. I think we can move __kernel_fsid_t into include/uapi/asm-generic/statfs.h, which is the only thing that needs it anyway. We have two definitions of it today, the non-generic one being for mips32, but incidentally there was a patch the other day to remove that and use the generic one instead. With that done, we can change asm/socket.h to just use asm/posix_types.h. I would still prefer to solve the problem for linux/posix_types.h as well, but I'm not sure even how __kernel_fd_set is used today in user space, if at all. Commit 8ded2bbc1845 ("posix_types.h: Cleanup stale __NFDBITS and related definitions") removed most of the fd_set definition after a long discussion [1], and since then it has been basically impossible to use 'struct fd_set' from the kernel in a meaningful way without including the libc headers or duplicating them. Should we just remove __kernel_fd_set from the exported headers and define the internal fd_set directly in include/linux/types.h? (Adding the folks from the old thread to Cc). Arnd [1] https://lore.kernel.org/lkml/20120724181209.GA10534@zod.bos.redhat.com/t/
* Arnd Bergmann: > Should we just remove __kernel_fd_set from the exported headers and > define the internal fd_set directly in include/linux/types.h? (Adding the > folks from the old thread to Cc). The type is used in the sanitizers, but incorrectly. They assume that FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is itself somewhat questionable because it leads to such bugs.) Moving around the type could cause a build failure in the sanitizers, but I'm not entirely clear how the UAPI headers are included there. Otherwise, I couldn't find any uses.
On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Arnd Bergmann: > > > Should we just remove __kernel_fd_set from the exported headers and > > define the internal fd_set directly in include/linux/types.h? (Adding the > > folks from the old thread to Cc). > > The type is used in the sanitizers, but incorrectly. They assume that > FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is > itself somewhat questionable because it leads to such bugs.) > Moving around the type could cause a build failure in the sanitizers, but I'm > not entirely clear how the UAPI headers are included there. It looks like sanitizer_platform_limits_posix.cc includes linux/posix_types.h to ensure that __kernel_fd_set is the same size as __sanitizer___kernel_fd_set, and then it uses the latter afterwards. What I don't see here is what kind of operation is actually done on the data, I only see a cast to void. If libsanitizer actually does anything interesting here, we should definitely fix it to use the correct size, especially since this is actually something that can trigger a buffer overflow in subtle ways when used carelessly. See for example [1], which we still have not addressed (I suspect we actually need to have glibc use __kernel_long_t instead of 'long int' here, but that is a separate issue, and not overly important given how few users there are on x32). For this specific use (and probably others like it), renaming the fds_bits member to __kernel_fds_bits or something like that would keep user space still compiling. That would only break if someone was using __kernel_fd_set, and actually doing bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN is set, so maybe we should use use that name unconditionally. > Otherwise, I couldn't find any uses.
* Arnd Bergmann: > On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote: >> >> * Arnd Bergmann: >> >> > Should we just remove __kernel_fd_set from the exported headers and >> > define the internal fd_set directly in include/linux/types.h? (Adding the >> > folks from the old thread to Cc). >> >> The type is used in the sanitizers, but incorrectly. They assume that >> FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is >> itself somewhat questionable because it leads to such bugs.) >> Moving around the type could cause a build failure in the sanitizers, but I'm >> not entirely clear how the UAPI headers are included there. > > It looks like sanitizer_platform_limits_posix.cc includes > linux/posix_types.h to ensure that __kernel_fd_set is the same > size as __sanitizer___kernel_fd_set, and then it uses the > latter afterwards. > > What I don't see here is what kind of operation is actually done > on the data, I only see a cast to void. I think it is used to assert that the select family of system calls writes to the 1024 bits for each of the passed pointers. Which is not actually true—the write size is controlled by the file descriptor count argument. > If libsanitizer actually does > anything interesting here, we should definitely fix it to use the > correct size, especially since this is actually something that > can trigger a buffer overflow in subtle ways when used carelessly. > See for example [1], which we still have not addressed The footnote is missing. > For this specific use (and probably others like it), renaming the > fds_bits member to __kernel_fds_bits or something like that > would keep user space still compiling. That would only break > if someone was using __kernel_fd_set, and actually doing > bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN > is set, so maybe we should use use that name unconditionally. Please use something that is more obviously Linux-specific.
On Mon, Mar 18, 2019 at 2:12 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote: > >> > >> * Arnd Bergmann: > >> > >> > Should we just remove __kernel_fd_set from the exported headers and > >> > define the internal fd_set directly in include/linux/types.h? (Adding the > >> > folks from the old thread to Cc). > >> > >> The type is used in the sanitizers, but incorrectly. They assume that > >> FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is > >> itself somewhat questionable because it leads to such bugs.) > >> Moving around the type could cause a build failure in the sanitizers, but I'm > >> not entirely clear how the UAPI headers are included there. > > > > It looks like sanitizer_platform_limits_posix.cc includes > > linux/posix_types.h to ensure that __kernel_fd_set is the same > > size as __sanitizer___kernel_fd_set, and then it uses the > > latter afterwards. > > > > What I don't see here is what kind of operation is actually done > > on the data, I only see a cast to void. > > I think it is used to assert that the select family of system calls > writes to the 1024 bits for each of the passed pointers. Yes, that is what I expected to see in libsanitizer, I just couldn't find any code that actually does this check. > Which is not actually true—the write size is controlled by the > file descriptor count argument. Yes, of course. In fact, I see multiple possible problems that - kernel reading uninitialized data if 'FD_ZERO()' was used with a shorter size than the count argument. - kernel writing beyond the fd_set data on stack when the declaration had a shorter size than the count argument. Each one could happen either because __FD_SETSIZE is smaller than 'count', or because kernel and user space disagree on the element size (32 vs 64 bit on x32). > > If libsanitizer actually does > > anything interesting here, we should definitely fix it to use the > > correct size, especially since this is actually something that > > can trigger a buffer overflow in subtle ways when used carelessly. > > See for example [1], which we still have not addressed > > The footnote is missing. Sorry, I meant [1] https://patchwork.kernel.org/patch/10245053/ > > For this specific use (and probably others like it), renaming the > > fds_bits member to __kernel_fds_bits or something like that > > would keep user space still compiling. That would only break > > if someone was using __kernel_fd_set, and actually doing > > bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN > > is set, so maybe we should use use that name unconditionally. > > Please use something that is more obviously Linux-specific. Ok, so not '__fds_bits'. Is '__kernel_fds_bits' ok? I would prefer to keep at least the name __kernel_ namespace that we have for typedefs and the occasional struct tag. Arnd
* Arnd Bergmann: > Ok, so not '__fds_bits'. > > Is '__kernel_fds_bits' ok? I would prefer to keep at least the > name __kernel_ namespace that we have for typedefs and the > occasional struct tag. glibc should be okay with that. We use __kernel_ in the math libraries for something completely different, but those files do not (or should not) include UAPI headers, and in any case, the set of such identifiers is really small.
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h index 0d0fddb7e738..976e89b116e5 100644 --- a/arch/alpha/include/uapi/asm/socket.h +++ b/arch/alpha/include/uapi/asm/socket.h @@ -2,8 +2,8 @@ #ifndef _UAPI_ASM_SOCKET_H #define _UAPI_ASM_SOCKET_H +#include <linux/posix_types.h> #include <asm/sockios.h> -#include <asm/bitsperlong.h> /* For setsockopt(2) */ /* diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h index eb9f33f8a8b3..d41765cfbc6e 100644 --- a/arch/mips/include/uapi/asm/socket.h +++ b/arch/mips/include/uapi/asm/socket.h @@ -10,8 +10,8 @@ #ifndef _UAPI_ASM_SOCKET_H #define _UAPI_ASM_SOCKET_H +#include <linux/posix_types.h> #include <asm/sockios.h> -#include <asm/bitsperlong.h> /* * For setsockopt(2) diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h index 16e428f03526..66c5dd245ac7 100644 --- a/arch/parisc/include/uapi/asm/socket.h +++ b/arch/parisc/include/uapi/asm/socket.h @@ -2,8 +2,8 @@ #ifndef _UAPI_ASM_SOCKET_H #define _UAPI_ASM_SOCKET_H +#include <linux/posix_types.h> #include <asm/sockios.h> -#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 0xffff diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h index 88fe4f978aca..9265a9eece15 100644 --- a/arch/sparc/include/uapi/asm/socket.h +++ b/arch/sparc/include/uapi/asm/socket.h @@ -2,8 +2,8 @@ #ifndef _ASM_SOCKET_H #define _ASM_SOCKET_H +#include <linux/posix_types.h> #include <asm/sockios.h> -#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 0xffff diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h index c8b430cb6dc4..8c1391c89171 100644 --- a/include/uapi/asm-generic/socket.h +++ b/include/uapi/asm-generic/socket.h @@ -2,8 +2,8 @@ #ifndef __ASM_GENERIC_SOCKET_H #define __ASM_GENERIC_SOCKET_H +#include <linux/posix_types.h> #include <asm/sockios.h> -#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 1
Referencing the __kernel_long_t type caused some user space applications to stop compiling when they had not already included linux/posix_types.h, e.g. s/multicast.c -o ext/sockets/multicast.lo In file included from /builddir/build/BUILD/php-7.3.3/main/php.h:468, from /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c:27: /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c: In function 'zm_startup_sockets': /builddir/build/BUILD/php-7.3.3/ext/sockets/sockets.c:776:40: error: '__kernel_long_t' undeclared (first use in this function) 776 | REGISTER_LONG_CONSTANT("SO_SNDTIMEO", SO_SNDTIMEO, CONST_CS | CONST_PERSISTENT); It is safe to include that header here, since it only contains kernel internal types that do not conflict with other user space types. It's still possible that some related build failures remain, but those are likely to be for code that is not already y2038 safe. Reported-by: Laura Abbott <labbott@redhat.com> Fixes: a9beb86ae6e5 ("sock: Add SO_RCVTIMEO_NEW and SO_SNDTIMEO_NEW") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/alpha/include/uapi/asm/socket.h | 2 +- arch/mips/include/uapi/asm/socket.h | 2 +- arch/parisc/include/uapi/asm/socket.h | 2 +- arch/sparc/include/uapi/asm/socket.h | 2 +- include/uapi/asm-generic/socket.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) -- 2.20.0