Re: [PATCH] librt: add test (bug 28213)

["Никита Попов" <npv1310@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9164 bytes --]

Thank you for your feedback!
I'm submitting a fixed version. It has slightly changed logic though.
The results are:
(before)
# cat rt/tst-bz28213.test-result
FAIL: rt/tst-bz28213
original exit status 1
# cat rt/tst-bz28213.out
Didn't expect signal from child: got `Segmentation fault'
(after)
# cat rt/tst-bz28213.test-result
PASS: rt/tst-bz28213
original exit status 0
# cat rt/tst-bz28213.out

чт, 12 авг. 2021 г. в 09:19, Siddhesh Poyarekar <siddhesh@gotplt.org>:
>
> On 8/11/21 4:56 PM, Никита Попов via Libc-alpha wrote:
> > Hello. I'm submitting a test case for recent bug 28213. It shows the
> > following results.
> > Before fix of bug 28213:
> > # cat ./rt/tst-bz28213.out
> > Child caused segmentation fault
> > # cat ./rt/tst-bz28213.test-result
> > FAIL: rt/tst-bz28213
> > original exit status 1
> >
> > After fix of bug 28213:
> > # cat ./rt/tst-bz28213.out
> > # cat ./rt/tst-bz28213.test-result
> > PASS: rt/tst-bz28213
> > original exit status 0
> > I managed to get libc cause segmentation fault.
> > Looking forward to receiving feedback from you.
>
> Thank you for sharing this.  My detailed review comments are inline.
> Overall I think this can be done without forking a new process since the
> test infrastructure already does that and is capable of reporting
> unexpected signals.  All you need to test is that the program runs to
> completion and ensure that the helper thread is called, perhaps by
> simply sleeping for a while.
>
> > From eb51dcf05befe6e839102e0cb622b1afdbf72a7a Mon Sep 17 00:00:00 2001
> > From: Nikita Popov <npv1310@gmail.com>
> > Date: Wed, 11 Aug 2021 14:36:50 +0500
> > Subject: [PATCH] librt: add test (bug 28213)
> > To: libc-alpha@sourceware.org
> >
> > This test implements following logic:
> > 1) Create dummy message queue, register it with mq_notify (using NULL attributes),
> >    immediately close the queue.
> >    Helper thread should cause NULL pointer dereference by this moment.
> > 2) Create another queue and try to send a message via the helper thread.
> >    Test is considered successful if the callback function receives the same message as was sent.
> >
> > Signed-off-by: Nikita Popov <npv1310@gmail.com>
> > ---
> >  rt/Makefile      |   1 +
> >  rt/tst-bz28213.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 175 insertions(+)
> >  create mode 100644 rt/tst-bz28213.c
> >
> > diff --git a/rt/Makefile b/rt/Makefile
> > index 113cea03a5..910e775995 100644
> > --- a/rt/Makefile
> > +++ b/rt/Makefile
> > @@ -74,6 +74,7 @@ tests := tst-shm tst-timer tst-timer2 \
> >        tst-aio7 tst-aio8 tst-aio9 tst-aio10 \
> >        tst-mqueue1 tst-mqueue2 tst-mqueue3 tst-mqueue4 \
> >        tst-mqueue5 tst-mqueue6 tst-mqueue7 tst-mqueue8 tst-mqueue9 \
> > +      tst-bz28213 \
> >        tst-timer3 tst-timer4 tst-timer5 \
> >        tst-cpuclock2 tst-cputimer1 tst-cputimer2 tst-cputimer3 \
> >        tst-shm-cancel \
> > diff --git a/rt/tst-bz28213.c b/rt/tst-bz28213.c
> > new file mode 100644
> > index 0000000000..6c043a9017
> > --- /dev/null
> > +++ b/rt/tst-bz28213.c
> > @@ -0,0 +1,174 @@
> > +/* Bug 28213: test for NULL pointer dereference in mq_notify.
> > +   Copyright (C) 2018-2021 Free Software Foundation, Inc.
>
> Please make this copyright "The GNU Toolchain Authors" since you don't
> have a copyright assignment with the FSF.
>
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <errno.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/wait.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <mqueue.h>
> > +#include <signal.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <support/xunistd.h>
> > +
> > +static const char check_bz28213_name[] = "/bz28213_queue";
> > +static const char check_bz28213_msg[] = "dummy";
> > +
> > +static void
> > +check_bz28213_cb (union sigval sv)
> > +{
> > +  char buf[sizeof (check_bz28213_msg)];
> > +  mqd_t m = sv.sival_int;
> > +  ssize_t n;
> > +
> > +  if ((n = mq_receive (m, buf, sizeof (buf), NULL)) < 0L ||
> > +      (size_t) n != sizeof (buf))
> > +    exit (1);
> > +
> > +  if (memcmp (buf, check_bz28213_msg, sizeof (buf)) != 0)
> > +    exit (1);
> > +
> > +  exit (0);
> > +}
>
> Instead of exiting with error, please use the TEST_VERIFY_EXIT macro in
> support/check.h.
>
> > +
> > +static void
> > +check_bz28213 (void)
> > +{
> > +  mqd_t m;
> > +  struct sigevent sev;
> > +  struct mq_attr attr;
> > +  unsigned int i;
> > +
> > +  /* First iteration should lead to undefined behavior due to NULL pointer dereference.
> > +     Second iteration tests whether helper thread still works. */
> > +  for (i = 0U; i < 2U; i++)
> > +    {
> > +      memset (&attr, '\0', sizeof (attr));
> > +      attr.mq_maxmsg = 1;
> > +      attr.mq_msgsize = sizeof (check_bz28213_msg);
> > +
> > +      if ((m = mq_open (check_bz28213_name,
> > +                        O_RDWR | O_CREAT | O_EXCL,
> > +                        0600,
> > +                        &attr)) < 0)
> > +        exit (1);
> > +
> > +      if (mq_unlink (check_bz28213_name) < 0)
> > +        exit (1);
> > +
> > +      memset (&sev, '\0', sizeof (sev));
> > +      sev.sigev_notify = SIGEV_THREAD;
> > +      sev.sigev_value.sival_int = m;
> > +      sev.sigev_notify_function = check_bz28213_cb;
> > +
> > +      if (mq_notify (m, &sev) < 0)
> > +        exit (1);
> > +
> > +      if (i && mq_send (m, check_bz28213_msg, sizeof (check_bz28213_msg), 1) < 0)
> > +        exit (1);
>
> Likewise for these exits, please use TEST_VERIFY_EXIT instead.
>
> > +
> > +      if (!i)
> > +        mq_close (m);
> > +    }
> > +
> > +  alarm (10);
> > +
> > +  while (1)
> > +    pause ();
>
> Instead you could define TIMEOUT to DEFAULT_TIMEOUT and then use a
> sleep(DEFAULT_TIMEOUT / 2) to wait long enough (10 seconds ought to be
> enough for anybody?) for the helper thread to run.  Architecture
> maintainers can later bump up TIMEOUT if they find that the test times
> out on their architecture.
>
> > +}
> > +
> > +/* Skip entire testing if queues are not implemented */
> > +static int
> > +check_mq_api (void)
> > +{
> > +  struct mq_attr attr;
> > +  mqd_t m;
> > +  int rc = 0;
> > +
> > +  memset (&attr, '\0', sizeof (attr));
> > +  attr.mq_maxmsg = 1;
> > +  attr.mq_msgsize = 1;
> > +
> > +  if ((m = mq_open (check_bz28213_name,
> > +                    O_RDWR | O_CREAT | O_EXCL,
> > +                    0600,
> > +                    &attr)) < 0)
> > +    {
> > +      if (errno == ENOSYS)
> > +        {
> > +          rc = 1;
> > +          printf ("SKIP: not implemented\n");
> > +        }
> > +
> > +      return rc;
> > +    }
> > +
> > +  mq_unlink (check_bz28213_name);
> > +  mq_close (m);
> > +
> > +  return rc;
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  pid_t pid;
> > +  int status, rc;
> > +
> > +  if (check_mq_api () == 1)
> > +    return 0;
>
> Please use FAIL_UNSUPPORTED() here.
>
> > +
> > +  if ((pid = xfork ()) == 0)
> > +    check_bz28213 ();
> > +
> > +  if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
> > +    {
> > +      kill (pid, SIGKILL);
> > +      printf ("waitpid failed\n");
> > +      return 1;
> > +    }
> > +
> > +  rc = 1;
> > +  if (WIFEXITED (status))
> > +    {
> > +      int child_rc;
> > +
> > +      if ((child_rc = WEXITSTATUS (status)) == 0)
> > +        rc = 0;
> > +      else
> > +        printf ("Child returned non-zero exit code [%d]\n", child_rc);
> > +    }
> > +  else if (WIFSIGNALED (status))
> > +    {
> > +      int child_signo;
> > +
> > +      if ((child_signo = WTERMSIG (status)) == SIGABRT)
> > +        printf ("Child timed out\n");
> > +      else if (child_signo == SIGSEGV)
> > +        printf ("Child caused segmentation fault\n");
> > +      else
> > +        printf ("Child terminated with signal %d\n", child_signo);
> > +    }
>
> Forking should not be necessary since the test driver already reports
> unexpected signals.
>
> > +
> > +  return rc;
>
> Return 0 in the end.
>
> > +}
> > +
> > +#include <support/test-driver.c>
> > --
> > 2.17.1
>

[-- Attachment #2: 0001-librt-add-test-bug-28213.patch --]
[-- Type: text/x-patch, Size: 4508 bytes --]

From 35600a79df9f3dedaca35ad8ec050be80e13fb12 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npv1310@gmail.com>
Date: Thu, 12 Aug 2021 12:07:00 +0500
Subject: [PATCH] librt: add test (bug 28213)
To: libc-alpha@sourceware.org

This test implements following logic:
1) Create POSIX message queue.
   Register a notification with mq_notify (using NULL thread attributes).
   Then immediately unregister the notification with mq_notify.
   Helper thread in a vulnerable version of glibc should cause NULL pointer dereference after these steps.
2) Once again, register the same notification. Try to send a dummy message.
   Test is considered successful if the dummy message is successfully received by the callback function.

Signed-off-by: Nikita Popov <npv1310@gmail.com>
---
 rt/Makefile      |   1 +
 rt/tst-bz28213.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 rt/tst-bz28213.c

diff --git a/rt/Makefile b/rt/Makefile
index 113cea03a5..910e775995 100644
--- a/rt/Makefile
+++ b/rt/Makefile
@@ -74,6 +74,7 @@ tests := tst-shm tst-timer tst-timer2 \
 	 tst-aio7 tst-aio8 tst-aio9 tst-aio10 \
 	 tst-mqueue1 tst-mqueue2 tst-mqueue3 tst-mqueue4 \
 	 tst-mqueue5 tst-mqueue6 tst-mqueue7 tst-mqueue8 tst-mqueue9 \
+	 tst-bz28213 \
 	 tst-timer3 tst-timer4 tst-timer5 \
 	 tst-cpuclock2 tst-cputimer1 tst-cputimer2 tst-cputimer3 \
 	 tst-shm-cancel \
diff --git a/rt/tst-bz28213.c b/rt/tst-bz28213.c
new file mode 100644
index 0000000000..b020e3d7dc
--- /dev/null
+++ b/rt/tst-bz28213.c
@@ -0,0 +1,102 @@
+/* Bug 28213: test for NULL pointer dereference in mq_notify.
+   Copyright (C) 2021-2021 The GNU Toolchain Authors.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <mqueue.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/test-driver.h>
+#include <support/check.h>
+
+static mqd_t m = -1;
+static const char msg[] = "hello";
+
+static void
+check_bz28213_cb (union sigval sv)
+{
+  char buf[sizeof (msg)];
+
+  (void) sv;
+
+  TEST_VERIFY_EXIT ((size_t) mq_receive (m, buf, sizeof (buf), NULL) == sizeof (buf));
+  TEST_VERIFY_EXIT (memcmp (buf, msg, sizeof (buf)) == 0);
+
+  exit (0);
+}
+
+static void
+check_bz28213 (void)
+{
+  struct sigevent sev;
+
+  memset (&sev, '\0', sizeof (sev));
+  sev.sigev_notify = SIGEV_THREAD;
+  sev.sigev_notify_function = check_bz28213_cb;
+
+  /* Step 1: Register & unregister notifier.
+     Helper thread should receive NOTIFY_REMOVED notification.
+     In a vulnerable version of glibc, NULL pointer dereference follows... */
+  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
+  TEST_VERIFY_EXIT (mq_notify (m, NULL) == 0);
+
+  /* Step 2: Once again, register notification.
+     Try to send one message.
+     Test is considered successful, if the callback does exit (0). */
+  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
+  TEST_VERIFY_EXIT (mq_send (m, msg, sizeof (msg), 1) == 0);
+
+  /* Wait... */
+  sleep (DEFAULT_TIMEOUT);
+  FAIL_EXIT1 ("Operation timed out\n");
+}
+
+static int
+do_test (void)
+{
+  static const char m_name[] = "/bz28213_queue";
+  struct mq_attr m_attr;
+
+  memset (&m_attr, '\0', sizeof (m_attr));
+  m_attr.mq_maxmsg = 1;
+  m_attr.mq_msgsize = sizeof (msg);
+
+  m = mq_open (m_name,
+               O_RDWR | O_CREAT | O_EXCL,
+               0600,
+               &m_attr);
+
+  if (m < 0)
+    {
+      if (errno == ENOSYS)
+        FAIL_UNSUPPORTED ("POSIX message queues are not implemented\n");
+      FAIL_EXIT1 ("Failed to create POSIX message queue: %m\n");
+    }
+
+  TEST_VERIFY_EXIT (mq_unlink (m_name) == 0);
+
+  check_bz28213 ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.17.1