From: Arnd Bergmann <arnd@arndb.de>
To: Florian Weimer <fw@deneb.enyo.de>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
"David S . Miller" <davem@davemloft.net>,
Willem de Bruijn <willemb@google.com>,
alpha <linux-alpha@vger.kernel.org>,
linux-arch <linux-arch@vger.kernel.org>,
linux-mips@vger.kernel.org,
Parisc List <linux-parisc@vger.kernel.org>,
sparclinux <sparclinux@vger.kernel.org>,
Laura Abbott <labbott@redhat.com>,
Networking <netdev@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Josh Boyer <jwboyer@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Jeff Law <law@redhat.com>
Subject: Re: [PATCH] y2038: fix socket.h header inclusion
Date: Mon, 18 Mar 2019 15:34:17 +0100 [thread overview]
Message-ID: <CAK8P3a3X26niT8Y8mWCNXgcRkWhT=ADK-Tt2vjYz6SLj90shCQ@mail.gmail.com> (raw)
In-Reply-To: <877ecwwckm.fsf@mid.deneb.enyo.de>
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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de>
To: Florian Weimer <fw@deneb.enyo.de>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
"David S . Miller" <davem@davemloft.net>,
Willem de Bruijn <willemb@google.com>,
alpha <linux-alpha@vger.kernel.org>,
linux-arch <linux-arch@vger.kernel.org>,
linux-mips@vger.kernel.org,
Parisc List <linux-parisc@vger.kernel.org>,
sparclinux <sparclinux@vger.kernel.org>,
Laura Abbott <labbott@redhat.com>,
Networking <netdev@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Josh Boyer <jwboyer@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Jeff Law <law@redhat.com>
Subject: Re: [PATCH] y2038: fix socket.h header inclusion
Date: Mon, 18 Mar 2019 14:34:17 +0000 [thread overview]
Message-ID: <CAK8P3a3X26niT8Y8mWCNXgcRkWhT=ADK-Tt2vjYz6SLj90shCQ@mail.gmail.com> (raw)
In-Reply-To: <877ecwwckm.fsf@mid.deneb.enyo.de>
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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de>
To: Florian Weimer <fw@deneb.enyo.de>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
"David S . Miller" <davem@davemloft.net>,
Willem de Bruijn <willemb@google.com>,
alpha <linux-alpha@vger.kernel.org>,
linux-arch <linux-arch@vger.kernel.org>,
linux-mips@vger.kernel.org,
Parisc List <linux-parisc@vger.kernel.org>,
sparclinux <sparclinux@vger.kernel.org>,
Laura Abbott <labbott@redhat.com>,
Networking <netdev@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Josh Boyer <jwboyer@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Jeff Law <law@redhat.com>
Subject: Re: [PATCH] y2038: fix socket.h header inclusion
Date: Mon, 18 Mar 2019 15:34:17 +0100 [thread overview]
Message-ID: <CAK8P3a3X26niT8Y8mWCNXgcRkWhT=ADK-Tt2vjYz6SLj90shCQ@mail.gmail.com> (raw)
In-Reply-To: <877ecwwckm.fsf@mid.deneb.enyo.de>
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
next prev parent reply other threads:[~2019-03-18 14:34 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-11 15:38 [PATCH] y2038: fix socket.h header inclusion Arnd Bergmann
2019-03-11 15:38 ` Arnd Bergmann
2019-03-11 18:23 ` David Miller
2019-03-11 18:23 ` David Miller
2019-03-14 18:37 ` Florian Weimer
2019-03-14 18:37 ` Florian Weimer
2019-03-15 20:30 ` Arnd Bergmann
2019-03-15 20:30 ` Arnd Bergmann
2019-03-15 21:20 ` Florian Weimer
2019-03-15 21:20 ` Florian Weimer
2019-03-17 18:20 ` Deepa Dinamani
2019-03-17 18:20 ` Deepa Dinamani
2019-03-18 8:27 ` Arnd Bergmann
2019-03-18 8:27 ` Arnd Bergmann
2019-03-18 9:21 ` Florian Weimer
2019-03-18 9:21 ` Florian Weimer
2019-03-18 12:56 ` Arnd Bergmann
2019-03-18 12:56 ` Arnd Bergmann
2019-03-18 13:12 ` Florian Weimer
2019-03-18 13:12 ` Florian Weimer
2019-03-18 13:12 ` Florian Weimer
2019-03-18 14:34 ` Arnd Bergmann [this message]
2019-03-18 14:34 ` Arnd Bergmann
2019-03-18 14:34 ` Arnd Bergmann
2019-03-18 14:37 ` Florian Weimer
2019-03-18 14:37 ` Florian Weimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK8P3a3X26niT8Y8mWCNXgcRkWhT=ADK-Tt2vjYz6SLj90shCQ@mail.gmail.com' \
--to=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=deepa.kernel@gmail.com \
--cc=fw@deneb.enyo.de \
--cc=jwboyer@redhat.com \
--cc=labbott@redhat.com \
--cc=law@redhat.com \
--cc=linux-alpha@vger.kernel.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sparclinux@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.