diff mbox series

[4/6] sysvipc: Return EINVAL for invalid msgctl commands

Message ID 20200928144556.239160-4-adhemerval.zanella@linaro.org
State Superseded
Headers
Series [1/6] sysvipc: Fix SEM_STAT_ANY kernel argument pass [BZ #26637] |

Commit Message

Adhemerval Zanella Sept. 28, 2020, 2:45 p.m. UTC 
  It avoids regressions on possible future commands that might require
additional libc support.  The downside is new commands added by newer
kernels will need further glibc support.

Checked on x86_64-linux-gnu and i686-linux-gnu (Linux v4.15 and v5.4).
---
 sysdeps/unix/sysv/linux/msgctl.c | 40 ++++++++++++++++++++++++--------
 sysvipc/test-sysvmsg.c           |  5 ++++
 2 files changed, 35 insertions(+), 10 deletions(-)

Comments

Florian Weimer Sept. 29, 2020, 7:58 a.m. UTC | #1 
* Adhemerval Zanella via Libc-alpha:

> It avoids regressions on possible future commands that might require
> additional libc support.  The downside is new commands added by newer
> kernels will need further glibc support.

Looks okay to me for now.

I think eventually we need to rethink what we want to do about these,
for the architectures that don't need translation.

Thanks,
Florian
Adhemerval Zanella Sept. 29, 2020, 1:26 p.m. UTC | #2 
On 29/09/2020 04:58, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> It avoids regressions on possible future commands that might require
>> additional libc support.  The downside is new commands added by newer
>> kernels will need further glibc support.
> 
> Looks okay to me for now.
> 
> I think eventually we need to rethink what we want to do about these,
> for the architectures that don't need translation.

We already have __IPC_TIME64 to tell apart the ABI that require it, 
we might to put the switch only if it is set.  However, I am not sure
if it is really an improvement, since we will still need to revise
it for architecture with __IPC_TIME64 == 1 anyway.
Florian Weimer Sept. 29, 2020, 5:05 p.m. UTC | #3 
* Adhemerval Zanella:

> On 29/09/2020 04:58, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> It avoids regressions on possible future commands that might require
>>> additional libc support.  The downside is new commands added by newer
>>> kernels will need further glibc support.
>> 
>> Looks okay to me for now.
>> 
>> I think eventually we need to rethink what we want to do about these,
>> for the architectures that don't need translation.
>
> We already have __IPC_TIME64 to tell apart the ABI that require it, 
> we might to put the switch only if it is set.  However, I am not sure
> if it is really an improvement, since we will still need to revise
> it for architecture with __IPC_TIME64 == 1 anyway.

It's an improvement for users on architectures which can use this
facility.  I think we should not artificially hold back architectures
with full userspace/kernel alignment just because there are other
architectures out there which are more complicated.

Thanks,
Florian
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/msgctl.c b/sysdeps/unix/sysv/linux/msgctl.c
index a1f24ab242..5a5e7482f2 100644
--- a/sysdeps/unix/sysv/linux/msgctl.c
+++ b/sysdeps/unix/sysv/linux/msgctl.c
@@ -88,25 +88,45 @@  __msgctl64 (int msqid, int cmd, struct __msqid64_ds *buf)
 {
 #if __IPC_TIME64
   struct kernel_msqid64_ds ksemid, *arg = NULL;
-  if (buf != NULL)
+#else
+  msgctl_arg_t *arg;
+#endif
+
+  switch (cmd)
     {
-      /* This is a Linux extension where kernel returns a 'struct msginfo'
-	 instead.  */
-      if (cmd == IPC_INFO || cmd == MSG_INFO)
-	arg = (struct kernel_msqid64_ds *) buf;
-      else
+    case IPC_RMID:
+      arg = NULL;
+      break;
+
+    case IPC_SET:
+    case IPC_STAT:
+    case MSG_STAT:
+#if __IPC_TIME64
+      if (buf != NULL)
 	{
 	  msqid64_to_kmsqid64 (buf, &ksemid);
 	  arg = &ksemid;
 	}
-    }
 # ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
-  if (cmd == IPC_SET)
-    arg->msg_perm.mode *= 0x10000U;
+      if (cmd == IPC_SET)
+	arg->msg_perm.mode *= 0x10000U;
 # endif
 #else
-  msgctl_arg_t *arg = buf;
+      arg = buf;
 #endif
+      break;
+
+    case IPC_INFO:
+    case MSG_INFO:
+      /* This is a Linux extension where kernel returns a 'struct msginfo'
+	 instead.  */
+      arg = (__typeof__ (arg)) buf;
+      break;
+
+    default:
+      __set_errno (EINVAL);
+      return -1;
+    }
 
   int ret = msgctl_syscall (msqid, cmd, arg);
   if (ret < 0)
diff --git a/sysvipc/test-sysvmsg.c b/sysvipc/test-sysvmsg.c
index 84efdade5e..ada2881065 100644
--- a/sysvipc/test-sysvmsg.c
+++ b/sysvipc/test-sysvmsg.c
@@ -24,6 +24,8 @@ 
 #include <sys/ipc.h>
 #include <sys/msg.h>
 
+#include <test-sysvipc.h>
+
 #include <support/support.h>
 #include <support/check.h>
 #include <support/temp_file.h>
@@ -86,6 +88,9 @@  do_test (void)
       FAIL_EXIT1 ("msgget failed (errno=%d)", errno);
     }
 
+  TEST_COMPARE (msgctl (msqid, first_msg_invalid_cmd (), NULL), -1);
+  TEST_COMPARE (errno, EINVAL);
+
   /* Get message queue kernel information and do some sanity checks.  */
   struct msqid_ds msginfo;
   if (msgctl (msqid, IPC_STAT, &msginfo) == -1)