* [PATCH v2] fix stack memory content leak via UNAME26 @ 2012-10-09 22:54 Kees Cook 2012-10-10 20:46 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2012-10-09 22:54 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, Andi Kleen, Kees Cook, PaX Team Calling uname() with the UNAME26 personality set allows a leak of kernel stack contents. This fixes it by initializing the stack buffer to zero, defensively calculating the length of copy_to_user() call, and making the len argument unsigned. CVE-2012-0957 Reported-by: PaX Team <pageexec@freemail.hu> Cc: stable@vger.kernel.org Cc: Andi Kleen <ak@linux.intel.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- v2: - corrected credit to PaX Team. - moved stack clearing into if case to avoid penalty to common case. --- kernel/sys.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index c5cb5b9..2300248 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1265,13 +1265,13 @@ DECLARE_RWSEM(uts_sem); * Work around broken programs that cannot handle "Linux 3.0". * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40 */ -static int override_release(char __user *release, int len) +static int override_release(char __user *release, size_t len) { int ret = 0; - char buf[65]; if (current->personality & UNAME26) { - char *rest = UTS_RELEASE; + const char *rest = UTS_RELEASE; + char buf[65] = { 0 }; int ndots = 0; unsigned v; @@ -1283,7 +1283,9 @@ static int override_release(char __user *release, int len) rest++; } v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40; - snprintf(buf, len, "2.6.%u%s", v, rest); + snprintf(buf, sizeof(buf), "2.6.%u%s", v, rest); + if (sizeof(buf) < len) + len = sizeof(buf); ret = copy_to_user(release, buf, len); } return ret; -- 1.7.9.5 -- Kees Cook Chrome OS Security ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-09 22:54 [PATCH v2] fix stack memory content leak via UNAME26 Kees Cook @ 2012-10-10 20:46 ` Andrew Morton 2012-10-10 22:31 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2012-10-10 20:46 UTC (permalink / raw) To: Kees Cook; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Tue, 9 Oct 2012 15:54:01 -0700 Kees Cook <keescook@chromium.org> wrote: > Calling uname() with the UNAME26 personality set allows a leak of kernel > stack contents. This fixes it by initializing the stack buffer to zero, > defensively calculating the length of copy_to_user() call, and making > the len argument unsigned. > > ... > > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1265,13 +1265,13 @@ DECLARE_RWSEM(uts_sem); > * Work around broken programs that cannot handle "Linux 3.0". > * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40 > */ > -static int override_release(char __user *release, int len) > +static int override_release(char __user *release, size_t len) > { > int ret = 0; > - char buf[65]; > > if (current->personality & UNAME26) { > - char *rest = UTS_RELEASE; > + const char *rest = UTS_RELEASE; > + char buf[65] = { 0 }; > int ndots = 0; > unsigned v; > > @@ -1283,7 +1283,9 @@ static int override_release(char __user *release, int len) > rest++; > } > v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40; > - snprintf(buf, len, "2.6.%u%s", v, rest); > + snprintf(buf, sizeof(buf), "2.6.%u%s", v, rest); > + if (sizeof(buf) < len) > + len = sizeof(buf); > ret = copy_to_user(release, buf, len); > } > return ret; This looks unecessarily complicated. Is there a reason to be copying all 65 bytes out to userspace? If not, then couldn't we just do len = scnprintf(...); ret = copy_to_user(..., len + 1); ? (This code is application #11,493 for the sprintf_user() which we don't have) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-10 20:46 ` Andrew Morton @ 2012-10-10 22:31 ` Kees Cook 2012-10-10 22:46 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2012-10-10 22:31 UTC (permalink / raw) To: Andrew Morton; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Wed, Oct 10, 2012 at 1:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 9 Oct 2012 15:54:01 -0700 > Kees Cook <keescook@chromium.org> wrote: > >> Calling uname() with the UNAME26 personality set allows a leak of kernel >> stack contents. This fixes it by initializing the stack buffer to zero, >> defensively calculating the length of copy_to_user() call, and making >> the len argument unsigned. >> >> ... >> >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -1265,13 +1265,13 @@ DECLARE_RWSEM(uts_sem); >> * Work around broken programs that cannot handle "Linux 3.0". >> * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40 >> */ >> -static int override_release(char __user *release, int len) >> +static int override_release(char __user *release, size_t len) >> { >> int ret = 0; >> - char buf[65]; >> >> if (current->personality & UNAME26) { >> - char *rest = UTS_RELEASE; >> + const char *rest = UTS_RELEASE; >> + char buf[65] = { 0 }; >> int ndots = 0; >> unsigned v; >> >> @@ -1283,7 +1283,9 @@ static int override_release(char __user *release, int len) >> rest++; >> } >> v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40; >> - snprintf(buf, len, "2.6.%u%s", v, rest); >> + snprintf(buf, sizeof(buf), "2.6.%u%s", v, rest); >> + if (sizeof(buf) < len) >> + len = sizeof(buf); >> ret = copy_to_user(release, buf, len); >> } >> return ret; > > This looks unecessarily complicated. Is there a reason to be copying > all 65 bytes out to userspace? > > If not, then couldn't we just do > > len = scnprintf(...); > ret = copy_to_user(..., len + 1); > > ? As it is, nothing calls override_release with crazy "len" values, but, to make the code less fragile, there should be checking for sizeof(buf) vs len. In the patch I sent, bounding the sprintf was sizeof(buf), and the copy_to_user was bounded by effectively min(sizeof(buf), len). If you wanted to use scnprintf, you'd have to reorganize the checks and explicitly handle len == 0: if (!len) return -EFAULT; if (sizeof(buf) < len) len = sizeof(buf) len = scnprintf(buf, len, "2.6.%u%s", v, rest); ret = copy_to_user(release, buf, len + 1); > (This code is application #11,493 for the sprintf_user() which we don't have) Indeed. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-10 22:31 ` Kees Cook @ 2012-10-10 22:46 ` Andrew Morton 2012-10-10 23:36 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2012-10-10 22:46 UTC (permalink / raw) To: Kees Cook; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Wed, 10 Oct 2012 15:31:07 -0700 Kees Cook <keescook@chromium.org> wrote: > > This looks unecessarily complicated. Is there a reason to be copying > > all 65 bytes out to userspace? > > > > If not, then couldn't we just do > > > > len = scnprintf(...); > > ret = copy_to_user(..., len + 1); > > > > ? > > As it is, nothing calls override_release with crazy "len" values, but, > to make the code less fragile, there should be checking for > sizeof(buf) vs len. In the patch I sent, bounding the sprintf was > sizeof(buf), and the copy_to_user was bounded by effectively > min(sizeof(buf), len). If you wanted to use scnprintf, you'd have to > reorganize the checks and explicitly handle len == 0: > > if (!len) > return -EFAULT; > if (sizeof(buf) < len) > len = sizeof(buf) > len = scnprintf(buf, len, "2.6.%u%s", v, rest); > ret = copy_to_user(release, buf, len + 1); It would be pretty absurd for someone to call override_release() with len==0? All callers use sizeof() on some pretty well-defined array. So I'd have thought that something like --- a/kernel/sys.c~a +++ a/kernel/sys.c @@ -1265,7 +1265,7 @@ DECLARE_RWSEM(uts_sem); * Work around broken programs that cannot handle "Linux 3.0". * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40 */ -static int override_release(char __user *release, int len) +static int override_release(char __user *release, size_t len) { int ret = 0; char buf[65]; @@ -1274,6 +1274,7 @@ static int override_release(char __user char *rest = UTS_RELEASE; int ndots = 0; unsigned v; + size_t copy; while (*rest) { if (*rest == '.' && ++ndots >= 3) @@ -1283,8 +1284,9 @@ static int override_release(char __user rest++; } v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40; - snprintf(buf, len, "2.6.%u%s", v, rest); - ret = copy_to_user(release, buf, len); + copy = scnprintf(buf, min(len, sizeof(buf)), + "2.6.%u%s", v, rest); + ret = copy_to_user(release, buf, copy + 1); } return ret; } would suffice? Not a big deal I guess, but copying out stuff beyond the NUL is a bit odd. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-10 22:46 ` Andrew Morton @ 2012-10-10 23:36 ` Kees Cook 2012-10-10 23:44 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2012-10-10 23:36 UTC (permalink / raw) To: Andrew Morton; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Wed, Oct 10, 2012 at 3:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 10 Oct 2012 15:31:07 -0700 > Kees Cook <keescook@chromium.org> wrote: > >> > This looks unecessarily complicated. Is there a reason to be copying >> > all 65 bytes out to userspace? >> > >> > If not, then couldn't we just do >> > >> > len = scnprintf(...); >> > ret = copy_to_user(..., len + 1); >> > >> > ? >> >> As it is, nothing calls override_release with crazy "len" values, but, >> to make the code less fragile, there should be checking for >> sizeof(buf) vs len. In the patch I sent, bounding the sprintf was >> sizeof(buf), and the copy_to_user was bounded by effectively >> min(sizeof(buf), len). If you wanted to use scnprintf, you'd have to >> reorganize the checks and explicitly handle len == 0: >> >> if (!len) >> return -EFAULT; >> if (sizeof(buf) < len) >> len = sizeof(buf) >> len = scnprintf(buf, len, "2.6.%u%s", v, rest); >> ret = copy_to_user(release, buf, len + 1); > > It would be pretty absurd for someone to call override_release() with > len==0? All callers use sizeof() on some pretty well-defined array. > > So I'd have thought that something like > > --- a/kernel/sys.c~a > +++ a/kernel/sys.c > @@ -1265,7 +1265,7 @@ DECLARE_RWSEM(uts_sem); > * Work around broken programs that cannot handle "Linux 3.0". > * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40 > */ > -static int override_release(char __user *release, int len) > +static int override_release(char __user *release, size_t len) > { > int ret = 0; > char buf[65]; > @@ -1274,6 +1274,7 @@ static int override_release(char __user > char *rest = UTS_RELEASE; > int ndots = 0; > unsigned v; > + size_t copy; > > while (*rest) { > if (*rest == '.' && ++ndots >= 3) > @@ -1283,8 +1284,9 @@ static int override_release(char __user > rest++; > } > v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40; > - snprintf(buf, len, "2.6.%u%s", v, rest); > - ret = copy_to_user(release, buf, len); > + copy = scnprintf(buf, min(len, sizeof(buf)), > + "2.6.%u%s", v, rest); > + ret = copy_to_user(release, buf, copy + 1); > } > return ret; > } > > would suffice? > > Not a big deal I guess, but copying out stuff beyond the NUL is a bit odd. Sure, that looks good to me. Thanks! -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-10 23:36 ` Kees Cook @ 2012-10-10 23:44 ` Andrew Morton 2012-10-11 0:39 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2012-10-10 23:44 UTC (permalink / raw) To: Kees Cook; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Wed, 10 Oct 2012 16:36:44 -0700 Kees Cook <keescook@chromium.org> wrote: > > would suffice? > > > > Not a big deal I guess, but copying out stuff beyond the NUL is a bit odd. > > Sure, that looks good to me. I think I'll stick with your tested patch for now. Could you please add "clean that up" to your todo list? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fix stack memory content leak via UNAME26 2012-10-10 23:44 ` Andrew Morton @ 2012-10-11 0:39 ` Kees Cook 0 siblings, 0 replies; 7+ messages in thread From: Kees Cook @ 2012-10-11 0:39 UTC (permalink / raw) To: Andrew Morton; +Cc: Linus Torvalds, linux-kernel, Andi Kleen, PaX Team On Wed, Oct 10, 2012 at 4:44 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 10 Oct 2012 16:36:44 -0700 > Kees Cook <keescook@chromium.org> wrote: > >> > would suffice? >> > >> > Not a big deal I guess, but copying out stuff beyond the NUL is a bit odd. >> >> Sure, that looks good to me. > > I think I'll stick with your tested patch for now. Could you please > add "clean that up" to your todo list? Sure. I've sent a v3 now. It still needed to handle the (theoretical) len == 0 case, though. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-10-11 0:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-09 22:54 [PATCH v2] fix stack memory content leak via UNAME26 Kees Cook 2012-10-10 20:46 ` Andrew Morton 2012-10-10 22:31 ` Kees Cook 2012-10-10 22:46 ` Andrew Morton 2012-10-10 23:36 ` Kees Cook 2012-10-10 23:44 ` Andrew Morton 2012-10-11 0:39 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox