From 2f37aeb620c474b9e2a58106f820444f838ea432 Mon Sep 17 00:00:00 2001 From: Michael Kerrisk Date: Fri, 16 Oct 2020 11:24:25 +0200 Subject: [PATCH] seccomp_unotify.2: EXAMPLE: Improve allocation of response buffer From a conversation with Jann Horn: [[ >>>> struct seccomp_notif_resp *resp = malloc(sizes.seccomp_notif_resp); >>> >>> This should probably do something like max(sizes.seccomp_notif_resp, >>> sizeof(struct seccomp_notif_resp)) in case the program was built >>> against new UAPI headers that make struct seccomp_notif_resp big, but >>> is running under an old kernel where that struct is still smaller? >> >> I'm confused. Why? I mean, if the running kernel says that it expects >> a buffer of a certain size, and we allocate a buffer of that size, >> what's the problem? > > Because in userspace, we cast the result of malloc() to a "struct > seccomp_notif_resp *". If the kernel tells us that it expects a size > smaller than sizeof(struct seccomp_notif_resp), then we end up with a > pointer to a struct that consists partly of allocated memory, partly > of out-of-bounds memory, which is generally a bad idea - I'm not sure > whether the C standard permits that. And if userspace then e.g. > decides to access some member of that struct that is beyond what the > kernel thinks is the struct size, we get actual OOB memory accesses. Got it. (But gosh, this seems like a fragile API mess.) I added the following to the code: /* When allocating the response buffer, we must allow for the fact that the user-space binary may have been built with user-space headers where 'struct seccomp_notif_resp' is bigger than the response buffer expected by the (older) kernel. Therefore, we allocate a buffer that is the maximum of the two sizes. This ensures that if the supervisor places bytes into the response structure that are past the response size that the kernel expects, then the supervisor is not touching an invalid memory location. */ size_t resp_size = sizes.seccomp_notif_resp; if (sizeof(struct seccomp_notif_resp) > resp_size) resp_size = sizeof(struct seccomp_notif_resp); struct seccomp_notif_resp *resp = malloc(resp_size); ]] Reported-by: Jann Horn Signed-off-by: Michael Kerrisk --- man2/seccomp_unotify.2 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/man2/seccomp_unotify.2 b/man2/seccomp_unotify.2 index 6f2c35ac8..f4d91c89a 100644 --- a/man2/seccomp_unotify.2 +++ b/man2/seccomp_unotify.2 @@ -1280,7 +1280,20 @@ handleNotifications(int notifyFd) if (req == NULL) errExit("\etS: malloc"); - struct seccomp_notif_resp *resp = malloc(sizes.seccomp_notif_resp); + /* When allocating the response buffer, we must allow for the fact + that the user\-space binary may have been built with user\-space + headers where \(aqstruct seccomp_notif_resp\(aq is bigger than the + response buffer expected by the (older) kernel. Therefore, we + allocate a buffer that is the maximum of the two sizes. This + ensures that if the supervisor places bytes into the response + structure that are past the response size that the kernel expects, + then the supervisor is not touching an invalid memory location. */ + + size_t resp_size = sizes.seccomp_notif_resp; + if (sizeof(struct seccomp_notif_resp) > resp_size) + resp_size = sizeof(struct seccomp_notif_resp); + + struct seccomp_notif_resp *resp = malloc(resp_size); if (resp == NULL) errExit("\etS: malloc"); -- 2.11.4.GIT