Re: [PATCH] Implement leftpad syscall

[Scotty Bauer]

On 03/31/2016 04:33 PM, Richard Weinberger wrote:
> From: David Gstir 
> 
> Implement the leftpad() system call such that userspace,
> especially node.js applications, can in the near future directly
> use it and no longer depend on fragile npm packages.
> 
> Signed-off-by: David Gstir 
> Signed-off-by: Richard Weinberger 
> ---
>  arch/x86/entry/syscalls/syscall_64.tbl |  1 +
>  include/linux/syscalls.h   |  1 +
>  kernel/sys.c   | 35 
> ++
>  kernel/sys_ni.c|  1 +
>  4 files changed, 38 insertions(+)
> 
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl 
> b/arch/x86/entry/syscalls/syscall_64.tbl
> index cac6d17..f287712 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -335,6 +335,7 @@
>  326  common  copy_file_range sys_copy_file_range
>  327  64  preadv2 sys_preadv2
>  328  64  pwritev2sys_pwritev2
> +329  common  leftpad sys_leftpad
>  
>  #
>  # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d795472..a0850bb 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -898,4 +898,5 @@ asmlinkage long sys_copy_file_range(int fd_in, loff_t 
> __user *off_in,
>  
>  asmlinkage long sys_mlock2(unsigned long start, size_t len, int flags);
>  
> +asmlinkage long sys_leftpad(char *str, char pad, char *dst, size_t dst_len);
>  #endif
> diff --git a/kernel/sys.c b/kernel/sys.c
> index cf8ba54..e42d972 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2432,3 +2432,38 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo 
> __user *, info)
>   return 0;
>  }
>  #endif /* CONFIG_COMPAT */
> +
> +
> +SYSCALL_DEFINE4(leftpad, char *, src, char, pad, char *, dst, size_t, 
> dst_len)
> +{
> + char *buf;
> + long ret;
> + size_t len = strlen_user(src);
> + size_t pad_len = dst_len - len; 
> +
> + if (dst_len <= len || dst_len > 4096) {
> + return -EINVAL;
> + }
> +
> + buf = kmalloc(dst_len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + memset(buf, pad, pad_len);
> + ret = copy_from_user(buf + pad_len, src, len);
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = copy_to_user(dst, buf, dst_len);
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = pad_len;
> +out:
> + kfree(buf);
> + return ret;
> +}

This looks good, but since we want this to be as fast as possible we might just 
want to eliminate all
branches (Pesky bounds checks), and write directly into user memory to 
eliminate the pesky copy_from/copy_to. The second
idea would eliminate that slow kmalloc as well.

What do you think?

> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.
> index 2c5e3a8..262608d 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -175,6 +175,7 @@ cond_syscall(sys_setfsgid);
>  cond_syscall(sys_capget);
>  cond_syscall(sys_capset);
>  cond_syscall(sys_copy_file_range);
> +cond_syscall(sys_leftpad);
>  
>  /* arch-specific weak syscall entries */
>  cond_syscall(sys_pciconfig_read);
> 

Re: [PATCH] Implement leftpad syscall

[Scotty Bauer]

On 03/31/2016 04:33 PM, Richard Weinberger wrote:
> From: David Gstir 
> 
> Implement the leftpad() system call such that userspace,
> especially node.js applications, can in the near future directly
> use it and no longer depend on fragile npm packages.
> 
> Signed-off-by: David Gstir 
> Signed-off-by: Richard Weinberger 
> ---
>  arch/x86/entry/syscalls/syscall_64.tbl |  1 +
>  include/linux/syscalls.h   |  1 +
>  kernel/sys.c   | 35 
> ++
>  kernel/sys_ni.c|  1 +
>  4 files changed, 38 insertions(+)
> 
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl 
> b/arch/x86/entry/syscalls/syscall_64.tbl
> index cac6d17..f287712 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -335,6 +335,7 @@
>  326  common  copy_file_range sys_copy_file_range
>  327  64  preadv2 sys_preadv2
>  328  64  pwritev2sys_pwritev2
> +329  common  leftpad sys_leftpad
>  
>  #
>  # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d795472..a0850bb 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -898,4 +898,5 @@ asmlinkage long sys_copy_file_range(int fd_in, loff_t 
> __user *off_in,
>  
>  asmlinkage long sys_mlock2(unsigned long start, size_t len, int flags);
>  
> +asmlinkage long sys_leftpad(char *str, char pad, char *dst, size_t dst_len);
>  #endif
> diff --git a/kernel/sys.c b/kernel/sys.c
> index cf8ba54..e42d972 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2432,3 +2432,38 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo 
> __user *, info)
>   return 0;
>  }
>  #endif /* CONFIG_COMPAT */
> +
> +
> +SYSCALL_DEFINE4(leftpad, char *, src, char, pad, char *, dst, size_t, 
> dst_len)
> +{
> + char *buf;
> + long ret;
> + size_t len = strlen_user(src);
> + size_t pad_len = dst_len - len; 
> +
> + if (dst_len <= len || dst_len > 4096) {
> + return -EINVAL;
> + }
> +
> + buf = kmalloc(dst_len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + memset(buf, pad, pad_len);
> + ret = copy_from_user(buf + pad_len, src, len);
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = copy_to_user(dst, buf, dst_len);
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = pad_len;
> +out:
> + kfree(buf);
> + return ret;
> +}

This looks good, but since we want this to be as fast as possible we might just 
want to eliminate all
branches (Pesky bounds checks), and write directly into user memory to 
eliminate the pesky copy_from/copy_to. The second
idea would eliminate that slow kmalloc as well.

What do you think?

> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.
> index 2c5e3a8..262608d 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -175,6 +175,7 @@ cond_syscall(sys_setfsgid);
>  cond_syscall(sys_capget);
>  cond_syscall(sys_capset);
>  cond_syscall(sys_copy_file_range);
> +cond_syscall(sys_leftpad);
>  
>  /* arch-specific weak syscall entries */
>  cond_syscall(sys_pciconfig_read);
> 

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 05:25 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 6:11 PM, Scotty Bauer <sba...@eng.utah.edu> wrote:
>>
>> Yeah I had toyed with using hashes, I used hash_64 not md5 which is like 14
>> extra instructions or something.
> 
> That sounds fine. Anything that requires enough code to undo that it
> kind of defeats the purpose of a SROP should be enough. It's not about
> encryption, I'd just think that if you can force the buffer overflow
> while already in a signal handler, you'd want something that is at
> least *slightly* harder to defeat than a single "xor" instruction.
> 
>> It's not hard to implement So I can try it. When you say an extra hardening
>> mode do you mean hide it behind a sysctl or some sort of compile time CONFIG?
> 
> Since there already is a sysctl, I'd just assume that.
> 
> The important part is that the *default* value for that sysctl can't
> break real applications. I don't really count CRIU as a real app, if
> only because once you start doing checkpoint-restore you are going to
> do some amount of system maintenance anyway, so somebody doing CRIU is
> kind of expected to have a certain amount of system expertise, I would
> say.
> 
> But dosemu - or Wine - is very much something that "normal people" run
> - people who we do *not* expect to have to know about new sysctl's
> etc. They already have one (mmap at zero), but that is very directly
> related to what vm86 mode and Wine does, and people have had time to
> learn about it. Let's not add another.
> 
> So testing dosemu and wine would be good. I wonder what else has shown
> issues with signal stack layout changes. Debuggers and some JIT
> engines, I suspect.
> 
>   Linus
> 


Alright I'll test Wine/Mono, Dosemu, some random languages/debuggers see if
there is anything that breaks.

Thanks.

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 05:25 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 6:11 PM, Scotty Bauer  wrote:
>>
>> Yeah I had toyed with using hashes, I used hash_64 not md5 which is like 14
>> extra instructions or something.
> 
> That sounds fine. Anything that requires enough code to undo that it
> kind of defeats the purpose of a SROP should be enough. It's not about
> encryption, I'd just think that if you can force the buffer overflow
> while already in a signal handler, you'd want something that is at
> least *slightly* harder to defeat than a single "xor" instruction.
> 
>> It's not hard to implement So I can try it. When you say an extra hardening
>> mode do you mean hide it behind a sysctl or some sort of compile time CONFIG?
> 
> Since there already is a sysctl, I'd just assume that.
> 
> The important part is that the *default* value for that sysctl can't
> break real applications. I don't really count CRIU as a real app, if
> only because once you start doing checkpoint-restore you are going to
> do some amount of system maintenance anyway, so somebody doing CRIU is
> kind of expected to have a certain amount of system expertise, I would
> say.
> 
> But dosemu - or Wine - is very much something that "normal people" run
> - people who we do *not* expect to have to know about new sysctl's
> etc. They already have one (mmap at zero), but that is very directly
> related to what vm86 mode and Wine does, and people have had time to
> learn about it. Let's not add another.
> 
> So testing dosemu and wine would be good. I wonder what else has shown
> issues with signal stack layout changes. Debuggers and some JIT
> engines, I suspect.
> 
>   Linus
> 


Alright I'll test Wine/Mono, Dosemu, some random languages/debuggers see if
there is anything that breaks.

Thanks.

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 04:34 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 4:38 PM, Andy Lutomirski  wrote:
>>
>> Then there's an unanswered question: is this patch acceptable given
>> that it's an ABI break?  Security fixes are sometimes an exception to
>> the "no ABI breaks" rule, but it's by no means an automatic exception.
> 
> So there isn't any "no ABI break" rule - there is only a "does it
> break real applications" rule.
> 
> (This can also be re-stated as: "Talk is cheap", aka "reality trumps
> documentation".
> 
> Documentation is meaningless if it doesn't match reality, and what we
> actually *do* is what matters.
> 
> So the ABI isn't about some theoretical interface documentation, the
> ABI is about what people use and have tested.
> 
> On the one hand, that means that that our ABI is _stricter_ than any
> documentatiuon, and that "but we can make this change that breaks app
> XYZ, because XYZ is depending on undocumented behavior" is not an
> acceptable excuse.
> 
> But on the other hand it *also* means that since the ABI is about real
> programs, not theoretical issues, we can also change things as long as
> we don't actually break anything that people can notice and depend
> on).
> 
> And while *acute* security holes will be fixed regardless of ABI
> issues, something like this that is only hardening rather than fixing
> a particular security hole, really needs to not break any
> applications.
> 
> Because if it does break anything, it needs to be turned off by
> default. That's a hard rule. And since that would be largely defeating
> the whole point o fthe series, I think we really need to have made
> sure nothing breaks before a patch series like this can be accepted.
> 
> That said, if this is done right, I don't think it will break
> anything. CRIU may indeed be a special case, but CRIU isn't really a
> normal application, and the CRIU people may need to turn this off
> explicitly, if it does break.
> 
> But yes, dosemu needs to be tested, and needs to just continue
> working. But does dosemu actually create a signal stack, as opposed to
> just playing with one that has been created for it? I thought it was
> just the latter case, which should be ok even with a magic cookie in
> there.
> 
>Linus
> 


For what it's worth this series is breaking CRIU, I just tested:

root@node0:/mnt/criu# criu restore - -o restore.log --shell-job
root@node0:/mnt/criu# tail -3 /var/log/syslog
Mar 29 17:12:08 localhost kernel: [ 3554.625535] Possible exploit attempt or 
buggy program!
Mar 29 17:12:08 localhost kernel: [ 3554.625535] If you believe this is an 
error you can disable SROP  Protection by #echo 1 > 
/proc/sys/kernel/disable-srop-protection
Mar 29 17:12:08 localhost kernel: [ 3554.625545] test_[25305] bad frame in 
rt_sigreturn frame:0001e540 ip:7f561542cf20 sp:7ffe004ecfd8 
orax: in libc-2.19.so[7f561536c000+1bb0]
root@node0:/mnt/criu# echo 1 > /proc/sys/kernel/disable-srop-protection 
root@node0:/mnt/criu# criu restore - -o restore.log --shell-job
slept for one second
slept for one second
slept for one second
slept for one second
root@node0:/mnt/criu# 


I'm working on getting dosemu up and running-- are there any other applications
off the top of your head that I should be testing with?

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 04:34 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 4:38 PM, Andy Lutomirski  wrote:
>>
>> Then there's an unanswered question: is this patch acceptable given
>> that it's an ABI break?  Security fixes are sometimes an exception to
>> the "no ABI breaks" rule, but it's by no means an automatic exception.
> 
> So there isn't any "no ABI break" rule - there is only a "does it
> break real applications" rule.
> 
> (This can also be re-stated as: "Talk is cheap", aka "reality trumps
> documentation".
> 
> Documentation is meaningless if it doesn't match reality, and what we
> actually *do* is what matters.
> 
> So the ABI isn't about some theoretical interface documentation, the
> ABI is about what people use and have tested.
> 
> On the one hand, that means that that our ABI is _stricter_ than any
> documentatiuon, and that "but we can make this change that breaks app
> XYZ, because XYZ is depending on undocumented behavior" is not an
> acceptable excuse.
> 
> But on the other hand it *also* means that since the ABI is about real
> programs, not theoretical issues, we can also change things as long as
> we don't actually break anything that people can notice and depend
> on).
> 
> And while *acute* security holes will be fixed regardless of ABI
> issues, something like this that is only hardening rather than fixing
> a particular security hole, really needs to not break any
> applications.
> 
> Because if it does break anything, it needs to be turned off by
> default. That's a hard rule. And since that would be largely defeating
> the whole point o fthe series, I think we really need to have made
> sure nothing breaks before a patch series like this can be accepted.
> 
> That said, if this is done right, I don't think it will break
> anything. CRIU may indeed be a special case, but CRIU isn't really a
> normal application, and the CRIU people may need to turn this off
> explicitly, if it does break.
> 
> But yes, dosemu needs to be tested, and needs to just continue
> working. But does dosemu actually create a signal stack, as opposed to
> just playing with one that has been created for it? I thought it was
> just the latter case, which should be ok even with a magic cookie in
> there.
> 
>Linus
> 


For what it's worth this series is breaking CRIU, I just tested:

root@node0:/mnt/criu# criu restore - -o restore.log --shell-job
root@node0:/mnt/criu# tail -3 /var/log/syslog
Mar 29 17:12:08 localhost kernel: [ 3554.625535] Possible exploit attempt or 
buggy program!
Mar 29 17:12:08 localhost kernel: [ 3554.625535] If you believe this is an 
error you can disable SROP  Protection by #echo 1 > 
/proc/sys/kernel/disable-srop-protection
Mar 29 17:12:08 localhost kernel: [ 3554.625545] test_[25305] bad frame in 
rt_sigreturn frame:0001e540 ip:7f561542cf20 sp:7ffe004ecfd8 
orax: in libc-2.19.so[7f561536c000+1bb0]
root@node0:/mnt/criu# echo 1 > /proc/sys/kernel/disable-srop-protection 
root@node0:/mnt/criu# criu restore - -o restore.log --shell-job
slept for one second
slept for one second
slept for one second
slept for one second
root@node0:/mnt/criu# 


I'm working on getting dosemu up and running-- are there any other applications
off the top of your head that I should be testing with?

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 04:54 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 2:53 PM, Scott Bauer  wrote:
>>
>> These patches implement the necessary changes to generate a cookie
>> which will be placed above signal frame upon signal delivery to userland.
>> The cookie is generated using a per-process random value xor'd with
>> the address where the cookie will be stored on the stack.
> 
> Side note: wouldn't it be better to make the cookie something that
> doesn't make it trivial to figure out the random value in case you
> already have access to a signal stack?
> 
> Maybe there could be a stronger variation of this that makes the
> cookie be something like a single md5 round (not a full md5).
> Something fast, and not necessarily secure, but something that needs
> more than one single CPU instruction to figure out.
> 
> So you could do 4 32
> 
>  - the random value
>  - the low 32 bits of the address of the cookie
>  - the low 32 bits of the return point stack and instruction pointer
> 
> Yes, yes, md5 is not cryptographically secure, and making it a single
> iteration rather than the full four makes it even less so, but if the
> attacker can generate long arbitrary code, then the whole SROP is
> pointless to begin with, no?
> 

Yeah I had toyed with using hashes, I used hash_64 not md5 which is like 14
extra instructions or something. Anyway Daniel Micay pointed out we could use 
SipHash
https://131002.net/siphash/, but there's no siphash for me to use in the kernel
and I'm the *last* person on earth to start porting/implementing 'crypto' algos.

Anyway, we all sort of agreed that if you have enough arbitrary execution 
already
to cause a signal, leak the cookie, do some xor magic to get the per-process 
secret then you probably don't really need to SROP in your exploit. Although
you did mention an interesting attack which is force a signal then muck with
an existing legitimate frame, which I would like to protect against now.

> In contrast, with the plain xor, the SROP would be a trivial operation
> if you can just force it to happen within the context of a signal, so
> that you can just re-use the signal return stack as-is. But mixing in
> the returning IP and SP would make it *much* harder to use the
> sigreturn as an attack vector.
> 
> I realize that this would likely need to be a separate and non-default
> extra hardening mode, because there are *definitely* applications that
> take signals and then update the return address (maybe single-stepping
> over instructions etc). But for a *lot* of applications, signal return
> implies changing no signal state at all, and mixing in the returning
> IP and SP would seem to be a fundamentally stronger cookie.
> 
> No?

It's not hard to implement So I can try it. When you say an extra hardening
mode do you mean hide it behind a sysctl or some sort of compile time CONFIG?

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 04:54 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 2:53 PM, Scott Bauer  wrote:
>>
>> These patches implement the necessary changes to generate a cookie
>> which will be placed above signal frame upon signal delivery to userland.
>> The cookie is generated using a per-process random value xor'd with
>> the address where the cookie will be stored on the stack.
> 
> Side note: wouldn't it be better to make the cookie something that
> doesn't make it trivial to figure out the random value in case you
> already have access to a signal stack?
> 
> Maybe there could be a stronger variation of this that makes the
> cookie be something like a single md5 round (not a full md5).
> Something fast, and not necessarily secure, but something that needs
> more than one single CPU instruction to figure out.
> 
> So you could do 4 32
> 
>  - the random value
>  - the low 32 bits of the address of the cookie
>  - the low 32 bits of the return point stack and instruction pointer
> 
> Yes, yes, md5 is not cryptographically secure, and making it a single
> iteration rather than the full four makes it even less so, but if the
> attacker can generate long arbitrary code, then the whole SROP is
> pointless to begin with, no?
> 

Yeah I had toyed with using hashes, I used hash_64 not md5 which is like 14
extra instructions or something. Anyway Daniel Micay pointed out we could use 
SipHash
https://131002.net/siphash/, but there's no siphash for me to use in the kernel
and I'm the *last* person on earth to start porting/implementing 'crypto' algos.

Anyway, we all sort of agreed that if you have enough arbitrary execution 
already
to cause a signal, leak the cookie, do some xor magic to get the per-process 
secret then you probably don't really need to SROP in your exploit. Although
you did mention an interesting attack which is force a signal then muck with
an existing legitimate frame, which I would like to protect against now.

> In contrast, with the plain xor, the SROP would be a trivial operation
> if you can just force it to happen within the context of a signal, so
> that you can just re-use the signal return stack as-is. But mixing in
> the returning IP and SP would make it *much* harder to use the
> sigreturn as an attack vector.
> 
> I realize that this would likely need to be a separate and non-default
> extra hardening mode, because there are *definitely* applications that
> take signals and then update the return address (maybe single-stepping
> over instructions etc). But for a *lot* of applications, signal return
> implies changing no signal state at all, and mixing in the returning
> IP and SP would seem to be a fundamentally stronger cookie.
> 
> No?

It's not hard to implement So I can try it. When you say an extra hardening
mode do you mean hide it behind a sysctl or some sort of compile time CONFIG?

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 03:29 PM, Andy Lutomirski wrote:
> On Tue, Mar 29, 2016 at 12:53 PM, Scott Bauer  wrote:
>> Sigreturn-oriented programming is a new attack vector in userland
>> where an attacker crafts a fake signal frame on the stack and calls
>> sigreturn. The kernel will extract the fake signal frame, which
>> contains attacker controlled "saved" registers. The kernel will then
>> transfer control to the attacker controlled userland instruction pointer.
>>
>> To prevent SROP attacks the kernel needs to know or be able to dervive
>> whether a sigreturn it is processing is in response to a legitimate
>> signal the kernel previously delivered.
>>
>> Further information and test code can be found in Documentation/security
>> and this excellent article:
>> http://lwn.net/Articles/676803/
>>
>> These patches implement the necessary changes to generate a cookie
>> which will be placed above signal frame upon signal delivery to userland.
>> The cookie is generated using a per-process random value xor'd with
>> the address where the cookie will be stored on the stack.
>>
>> Upon a sigreturn the kernel will extract the cookie from userland,
>> recalculate what the original cookie should be and verify that the two
>> do not differ. If the two differ the kernel will terminate the process
>> with a SIGSEGV.
>>
>> This prevents SROP by adding a value that the attacker cannot guess,
>> but the kernel can verify. Therefore an attacker cannot use sigreturn as
>> a method to control the flow of a process.
>>
> 
> Has anyone verified that this doesn't break CRIU cross-machine (or
> cross-boot) migration and that this doesn't break dosemu?  You're
> changing the ABI here.
> 

I haven't yet I'll do that to verify it breaks -- I'm pretty sure under some
conditions it will break CRIU. That's why we added the sysctl to turn it off. 
Should I have mentioned this in the main commit that it possibly breaks 
CRIU/DOSEMU?
I went ahead and added that to the Documentation.

Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Scotty Bauer]

On 03/29/2016 03:29 PM, Andy Lutomirski wrote:
> On Tue, Mar 29, 2016 at 12:53 PM, Scott Bauer  wrote:
>> Sigreturn-oriented programming is a new attack vector in userland
>> where an attacker crafts a fake signal frame on the stack and calls
>> sigreturn. The kernel will extract the fake signal frame, which
>> contains attacker controlled "saved" registers. The kernel will then
>> transfer control to the attacker controlled userland instruction pointer.
>>
>> To prevent SROP attacks the kernel needs to know or be able to dervive
>> whether a sigreturn it is processing is in response to a legitimate
>> signal the kernel previously delivered.
>>
>> Further information and test code can be found in Documentation/security
>> and this excellent article:
>> http://lwn.net/Articles/676803/
>>
>> These patches implement the necessary changes to generate a cookie
>> which will be placed above signal frame upon signal delivery to userland.
>> The cookie is generated using a per-process random value xor'd with
>> the address where the cookie will be stored on the stack.
>>
>> Upon a sigreturn the kernel will extract the cookie from userland,
>> recalculate what the original cookie should be and verify that the two
>> do not differ. If the two differ the kernel will terminate the process
>> with a SIGSEGV.
>>
>> This prevents SROP by adding a value that the attacker cannot guess,
>> but the kernel can verify. Therefore an attacker cannot use sigreturn as
>> a method to control the flow of a process.
>>
> 
> Has anyone verified that this doesn't break CRIU cross-machine (or
> cross-boot) migration and that this doesn't break dosemu?  You're
> changing the ABI here.
> 

I haven't yet I'll do that to verify it breaks -- I'm pretty sure under some
conditions it will break CRIU. That's why we added the sysctl to turn it off. 
Should I have mentioned this in the main commit that it possibly breaks 
CRIU/DOSEMU?
I went ahead and added that to the Documentation.

Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

[Scotty Bauer]

On 03/29/2016 01:59 PM, Andi Kleen wrote:
> On Tue, Mar 29, 2016 at 01:53:26PM -0600, Scott Bauer wrote:
>> This patch adds a sysctl argument to disable SROP protection.
> 
> Sysctl needs to be documented in Documentation/sysctl/
> 
> Also negated sysctl is weird, normally they are positive (enable-xxx)
> 

Sure, I can change it. This may be a dumb question: I want SROP to be enabled 
by default, and thus the new
enable-xxx will be initialized to 1, that's fine, right?

Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

[Scotty Bauer]

On 03/29/2016 01:59 PM, Andi Kleen wrote:
> On Tue, Mar 29, 2016 at 01:53:26PM -0600, Scott Bauer wrote:
>> This patch adds a sysctl argument to disable SROP protection.
> 
> Sysctl needs to be documented in Documentation/sysctl/
> 
> Also negated sysctl is weird, normally they are positive (enable-xxx)
> 

Sure, I can change it. This may be a dumb question: I want SROP to be enabled 
by default, and thus the new
enable-xxx will be initialized to 1, that's fine, right?

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/09/2016 01:32 AM, Ingo Molnar wrote:
> 
> * Scott Bauer  wrote:
> 
>> This patch adds a per-process secret to the task struct which
>> will be used during signal delivery and during a sigreturn.
>> Also, logic is added in signal.c to generate, place, extract,
>> clear and verify the signal cookie.
> 
>>  /*
>> + * Canary value for signal frames placed on user stack.
>> + * This helps mitigate "Signal Return oriented program"
>> + * exploits in userland.
>> + */
>> +unsigned long sig_cookie;
> 
> Could you please add a high level description in Documentation
> that explains the attack and the way how this mitigation code
> prevents that kind of attack?
> 
> Also, the first changelogs should contain more high level
> description as well. For example, what does the 'verification'
> of the signal cookie mean, and how does it prevent an SROP
> attempt?
> 
> All of these patches seem to assume that people reading this code
> know what SROP is and how we defend against it - that is not so.
> 
> Thanks,
> 
>   Ingo
> 


I'm going to submit v4 to fix some nits where I'll include the explanation
and a change log, I apologize for not doing that here. In the meantime if
you don't mind visiting a link I included a brief explanation on previous
versions of the patch set.


https://lkml.org/lkml/2016/2/6/166

Thanks

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/09/2016 01:32 AM, Ingo Molnar wrote:
> 
> * Scott Bauer  wrote:
> 
>> This patch adds a per-process secret to the task struct which
>> will be used during signal delivery and during a sigreturn.
>> Also, logic is added in signal.c to generate, place, extract,
>> clear and verify the signal cookie.
> 
>>  /*
>> + * Canary value for signal frames placed on user stack.
>> + * This helps mitigate "Signal Return oriented program"
>> + * exploits in userland.
>> + */
>> +unsigned long sig_cookie;
> 
> Could you please add a high level description in Documentation
> that explains the attack and the way how this mitigation code
> prevents that kind of attack?
> 
> Also, the first changelogs should contain more high level
> description as well. For example, what does the 'verification'
> of the signal cookie mean, and how does it prevent an SROP
> attempt?
> 
> All of these patches seem to assume that people reading this code
> know what SROP is and how we defend against it - that is not so.
> 
> Thanks,
> 
>   Ingo
> 


I'm going to submit v4 to fix some nits where I'll include the explanation
and a change log, I apologize for not doing that here. In the meantime if
you don't mind visiting a link I included a brief explanation on previous
versions of the patch set.


https://lkml.org/lkml/2016/2/6/166

Thanks

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer <sba...@eng.utah.edu> wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <sba...@eng.utah.edu> wrote:
>>>> This patch adds a per-process secret to the task struct which
>>>> will be used during signal delivery and during a sigreturn.
>>>> Also, logic is added in signal.c to generate, place, extract,
>>>> clear and verify the signal cookie.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
> 
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil.  I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
> 
> --Andy
> 

I've been thinking about this a little bit more. I don't think zeroing the frame
is a proper mitigation. If an attacker has the ability to write a lot of data to
the stack they could simply create a new fake signal frame above the current 
frame.
In this scenario the kernel would zero the current frame then return somewhere 
attacker
controlled, where the attackers payload would then use the next signal frame 
above
the zero'd frame.

So while this zeroing would solve a stricter case where an attacker has to keep 
reusing
the same frame over and over, perhaps to avoid overwriting a stack cookie, It 
doesn't solve
every case. 

Thanks for the good ideas.

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer  wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer  wrote:
>>>> This patch adds a per-process secret to the task struct which
>>>> will be used during signal delivery and during a sigreturn.
>>>> Also, logic is added in signal.c to generate, place, extract,
>>>> clear and verify the signal cookie.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
> 
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil.  I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
> 
> --Andy
> 

I've been thinking about this a little bit more. I don't think zeroing the frame
is a proper mitigation. If an attacker has the ability to write a lot of data to
the stack they could simply create a new fake signal frame above the current 
frame.
In this scenario the kernel would zero the current frame then return somewhere 
attacker
controlled, where the attackers payload would then use the next signal frame 
above
the zero'd frame.

So while this zeroing would solve a stricter case where an attacker has to keep 
reusing
the same frame over and over, perhaps to avoid overwriting a stack cookie, It 
doesn't solve
every case. 

Thanks for the good ideas.

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer <sba...@eng.utah.edu> wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <sba...@eng.utah.edu> wrote:
>>>> This patch adds a per-process secret to the task struct which
>>>> will be used during signal delivery and during a sigreturn.
>>>> Also, logic is added in signal.c to generate, place, extract,
>>>> clear and verify the signal cookie.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
> 
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil.  I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
> 
> --Andy
> 

Ah, I see now. I believe that would work for subsequent sigreturns but not
the first.

The paper did talk about actually ROP'ing using sigreturns but I never really
liked that idea. I think they missed the obvious reason an attacker would use
SROP. The reason an attacker would use is it gives you an extremely easy
way to get values into registers.. you just set them to what you want them to be
then sigret. Previously an attacker would have to find gadgets and ROP around 
until
the registers are in a good enough state to do what ever they want. 

I mostly designed the patches with that in mind instead of actually ROPing with 
sigret.

I can certainly add zeroing the stack frame, but not sure if there would be a 
performance
regression in doing so or if it's really relevant if we keep the cookie.

 

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer  wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer  wrote:
>>>> This patch adds a per-process secret to the task struct which
>>>> will be used during signal delivery and during a sigreturn.
>>>> Also, logic is added in signal.c to generate, place, extract,
>>>> clear and verify the signal cookie.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
> 
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil.  I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
> 
> --Andy
> 

Ah, I see now. I believe that would work for subsequent sigreturns but not
the first.

The paper did talk about actually ROP'ing using sigreturns but I never really
liked that idea. I think they missed the obvious reason an attacker would use
SROP. The reason an attacker would use is it gives you an extremely easy
way to get values into registers.. you just set them to what you want them to be
then sigret. Previously an attacker would have to find gadgets and ROP around 
until
the registers are in a good enough state to do what ever they want. 

I mostly designed the patches with that in mind instead of actually ROPing with 
sigret.

I can certainly add zeroing the stack frame, but not sure if there would be a 
performance
regression in doing so or if it's really relevant if we keep the cookie.

 

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer  wrote:
>> This patch adds a per-process secret to the task struct which
>> will be used during signal delivery and during a sigreturn.
>> Also, logic is added in signal.c to generate, place, extract,
>> clear and verify the signal cookie.
>>
> 
> Potentially silly question: it's been a while since I read the SROP
> paper, but would the technique be effectively mitigated if sigreturn
> were to zero out the whole signal frame before returning to user mode?
> 

I don't know if I fully understand your question, but I'll respond anyway.

SROP is possible because the kernel doesn't know whether or not the
incoming sigreturn syscall is in response from a legitimate signal that
the kernel had previously delivered and the program handled. So essentially
these patches are an attempt to give the kernel a way to verify whether or 
not the the incoming sigreturn is a valid response or a exploit trying to
hijack control of the user program.

So no, zeroing out the frame wouldn't do much because if I understand your
question correctly once we call sigreturn the kernel is going to hand off
control to wherever the sigframe tells it to so I don't think zeroing would
do much.

The reason why I zero out the cookie is so if there is a stack leak bug or 
something along those lines an attacker couldnt leak the cookie and try and
derive what the per-process kernel secret is.

Hope that clarifies!

Re: [PATCH v3 1/3] SROP Mitigation: Architecture independent code for signal cookies

[Scotty Bauer]

On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer  wrote:
>> This patch adds a per-process secret to the task struct which
>> will be used during signal delivery and during a sigreturn.
>> Also, logic is added in signal.c to generate, place, extract,
>> clear and verify the signal cookie.
>>
> 
> Potentially silly question: it's been a while since I read the SROP
> paper, but would the technique be effectively mitigated if sigreturn
> were to zero out the whole signal frame before returning to user mode?
> 

I don't know if I fully understand your question, but I'll respond anyway.

SROP is possible because the kernel doesn't know whether or not the
incoming sigreturn syscall is in response from a legitimate signal that
the kernel had previously delivered and the program handled. So essentially
these patches are an attempt to give the kernel a way to verify whether or 
not the the incoming sigreturn is a valid response or a exploit trying to
hijack control of the user program.

So no, zeroing out the frame wouldn't do much because if I understand your
question correctly once we call sigreturn the kernel is going to hand off
control to wherever the sigframe tells it to so I don't think zeroing would
do much.

The reason why I zero out the cookie is so if there is a stack leak bug or 
something along those lines an attacker couldnt leak the cookie and try and
derive what the per-process kernel secret is.

Hope that clarifies!

Re: [PATCH v3 2/3] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 03/08/2016 02:03 PM, One Thousand Gnomes wrote:
>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>> -   struct sigcontext_32 __user *sc)
>> +   struct sigcontext_32 __user *sc,
>> +   void __user **user_cookie)
>>  {
>>  unsigned int tmpflags, err = 0;
>>  void __user *buf;
>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>>  buf = compat_ptr(tmp);
>>  } get_user_catch(err);
>>  
>> +/*
>> + * If there is fp state get cookie from the top of the fp state,
>> + * else get it from the top of the sig frame.
>> + */
>> +
>> +if (tmp != 0)
>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>> +else
>> +*user_cookie = NULL;
> 
> user_cookie is is __user, so shouldn't just be poking at it without
> get/put_user ? It might fault if someone has engineered a bad stack frame.
> 
> Alan
> 

I guess I got a little carried away with the __user annotations. I will remove 
the __user annotation from this function
since what we're dereferencing isn't actually a __user pointer, its some stack 
memory sitting in the caller's stack frame. 

see patch 2/3:


void __user *user_cookie;
  vv-- Passed in here
if (ia32_restore_sigcontext(regs, >sc, _cookie))
goto badframe;

Re: [PATCH v3 2/3] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 03/08/2016 02:03 PM, One Thousand Gnomes wrote:
>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>> -   struct sigcontext_32 __user *sc)
>> +   struct sigcontext_32 __user *sc,
>> +   void __user **user_cookie)
>>  {
>>  unsigned int tmpflags, err = 0;
>>  void __user *buf;
>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>>  buf = compat_ptr(tmp);
>>  } get_user_catch(err);
>>  
>> +/*
>> + * If there is fp state get cookie from the top of the fp state,
>> + * else get it from the top of the sig frame.
>> + */
>> +
>> +if (tmp != 0)
>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>> +else
>> +*user_cookie = NULL;
> 
> user_cookie is is __user, so shouldn't just be poking at it without
> get/put_user ? It might fault if someone has engineered a bad stack frame.
> 
> Alan
> 

I guess I got a little carried away with the __user annotations. I will remove 
the __user annotation from this function
since what we're dereferencing isn't actually a __user pointer, its some stack 
memory sitting in the caller's stack frame. 

see patch 2/3:


void __user *user_cookie;
  vv-- Passed in here
if (ia32_restore_sigcontext(regs, >sc, _cookie))
goto badframe;

Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 02/08/2016 02:50 PM, Andy Lutomirski wrote:
> On Sun, Feb 7, 2016 at 12:10 AM, Scotty Bauer  wrote:
>>
>>
>> On 02/06/2016 11:35 PM, Mika Penttilä wrote:
>>> Hi,
>>>
>>>
>>> On 07.02.2016 01:39, Scott Bauer wrote:
>>>> This patch adds SROP mitigation logic to the x86 signal delivery
>>>> and sigreturn code. The cookie is placed in the unused alignment
>>>> space above the saved FP state, if it exists. If there is no FP
>>>> state to save then the cookie is placed in the alignment space above
>>>> the sigframe.
>>>>
>>>> Cc: Abhiram Balasubramanian 
>>>> Signed-off-by: Scott Bauer 
>>>> ---
>>>>  arch/x86/ia32/ia32_signal.c| 63 +---
>>>>  arch/x86/include/asm/fpu/signal.h  |  1 +
>>>>  arch/x86/include/asm/sighandling.h |  5 ++-
>>>>  arch/x86/kernel/fpu/signal.c   | 10 +
>>>>  arch/x86/kernel/signal.c   | 86 
>>>> +-
>>>>  5 files changed, 146 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>>>> index 0552884..2751f47 100644
>>>> --- a/arch/x86/ia32/ia32_signal.c
>>>> +++ b/arch/x86/ia32/ia32_signal.c
>>>> @@ -68,7 +68,8 @@
>>>>  }
>>>>
>>>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>>>> -   struct sigcontext_32 __user *sc)
>>>> +   struct sigcontext_32 __user *sc,
>>>> +   void __user **user_cookie)
>>>>  {
>>>>  unsigned int tmpflags, err = 0;
>>>>  void __user *buf;
>>>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs 
>>>> *regs,
>>>>  buf = compat_ptr(tmp);
>>>>  } get_user_catch(err);
>>>>
>>>> +/*
>>>> + * If there is fp state get cookie from the top of the fp state,
>>>> + * else get it from the top of the sig frame.
>>>> + */
>>>> +
>>>> +if (tmp != 0)
>>>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>>>> +else
>>>> +*user_cookie = NULL;
>>>> +
>>>>  err |= fpu__restore_sig(buf, 1);
>>>>
>>>>  force_iret();
>>>> @@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
>>>>  struct pt_regs *regs = current_pt_regs();
>>>>  struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user 
>>>> *)(regs->sp-8);
>>>>  sigset_t set;
>>>> +void __user *user_cookie;
>>>>
>>>>  if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>  goto badframe;
>>>> @@ -129,8 +141,15 @@ asmlinkage long sys32_sigreturn(void)
>>>>
>>>>  set_current_blocked();
>>>>
>>>> -if (ia32_restore_sigcontext(regs, >sc))
>>>> +if (ia32_restore_sigcontext(regs, >sc, _cookie))
>>>> +goto badframe;
>>>> +
>>>> +if (user_cookie == NULL)
>>>> +user_cookie = compat_ptr((regs->sp - 8) + sizeof(*frame));
>>>> +
>>>> +if (verify_clear_sigcookie(user_cookie))
>>>>  goto badframe;
>>>> +
>>>>  return regs->ax;
>>>>
>>>>  badframe:
>>>> @@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
>>>>  {
>>>>  struct pt_regs *regs = current_pt_regs();
>>>>  struct rt_sigframe_ia32 __user *frame;
>>>> +void __user *user_cookie;
>>>>  sigset_t set;
>>>>
>>>>  frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
>>>> @@ -153,7 +173,13 @@ asmlinkage long sys32_rt_sigreturn(void)
>>>>
>>>>  set_current_blocked();
>>>>
>>>> -if (ia32_restore_sigcontext(regs, >uc.uc_mcontext))
>>>> +if (ia32_restore_sigcontext(regs, >uc.uc_mcontext, 
>>>> _cookie))
>>>> +goto badframe;
>>>> +
>>>> +if (user_cookie == NULL)
>>>> +user_cookie = (void __user *)((regs->sp - 4) + 
>>>> sizeof(*frame));
>>> regs->sp is already restored so yo

Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 02/08/2016 02:50 PM, Andy Lutomirski wrote:
> On Sun, Feb 7, 2016 at 12:10 AM, Scotty Bauer <sba...@eng.utah.edu> wrote:
>>
>>
>> On 02/06/2016 11:35 PM, Mika Penttilä wrote:
>>> Hi,
>>>
>>>
>>> On 07.02.2016 01:39, Scott Bauer wrote:
>>>> This patch adds SROP mitigation logic to the x86 signal delivery
>>>> and sigreturn code. The cookie is placed in the unused alignment
>>>> space above the saved FP state, if it exists. If there is no FP
>>>> state to save then the cookie is placed in the alignment space above
>>>> the sigframe.
>>>>
>>>> Cc: Abhiram Balasubramanian <abhi...@cs.utah.edu>
>>>> Signed-off-by: Scott Bauer <sba...@eng.utah.edu>
>>>> ---
>>>>  arch/x86/ia32/ia32_signal.c| 63 +---
>>>>  arch/x86/include/asm/fpu/signal.h  |  1 +
>>>>  arch/x86/include/asm/sighandling.h |  5 ++-
>>>>  arch/x86/kernel/fpu/signal.c   | 10 +
>>>>  arch/x86/kernel/signal.c   | 86 
>>>> +-
>>>>  5 files changed, 146 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>>>> index 0552884..2751f47 100644
>>>> --- a/arch/x86/ia32/ia32_signal.c
>>>> +++ b/arch/x86/ia32/ia32_signal.c
>>>> @@ -68,7 +68,8 @@
>>>>  }
>>>>
>>>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>>>> -   struct sigcontext_32 __user *sc)
>>>> +   struct sigcontext_32 __user *sc,
>>>> +   void __user **user_cookie)
>>>>  {
>>>>  unsigned int tmpflags, err = 0;
>>>>  void __user *buf;
>>>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs 
>>>> *regs,
>>>>  buf = compat_ptr(tmp);
>>>>  } get_user_catch(err);
>>>>
>>>> +/*
>>>> + * If there is fp state get cookie from the top of the fp state,
>>>> + * else get it from the top of the sig frame.
>>>> + */
>>>> +
>>>> +if (tmp != 0)
>>>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>>>> +else
>>>> +*user_cookie = NULL;
>>>> +
>>>>  err |= fpu__restore_sig(buf, 1);
>>>>
>>>>  force_iret();
>>>> @@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
>>>>  struct pt_regs *regs = current_pt_regs();
>>>>  struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user 
>>>> *)(regs->sp-8);
>>>>  sigset_t set;
>>>> +void __user *user_cookie;
>>>>
>>>>  if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>  goto badframe;
>>>> @@ -129,8 +141,15 @@ asmlinkage long sys32_sigreturn(void)
>>>>
>>>>  set_current_blocked();
>>>>
>>>> -if (ia32_restore_sigcontext(regs, >sc))
>>>> +if (ia32_restore_sigcontext(regs, >sc, _cookie))
>>>> +goto badframe;
>>>> +
>>>> +if (user_cookie == NULL)
>>>> +user_cookie = compat_ptr((regs->sp - 8) + sizeof(*frame));
>>>> +
>>>> +if (verify_clear_sigcookie(user_cookie))
>>>>  goto badframe;
>>>> +
>>>>  return regs->ax;
>>>>
>>>>  badframe:
>>>> @@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
>>>>  {
>>>>  struct pt_regs *regs = current_pt_regs();
>>>>  struct rt_sigframe_ia32 __user *frame;
>>>> +void __user *user_cookie;
>>>>  sigset_t set;
>>>>
>>>>  frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
>>>> @@ -153,7 +173,13 @@ asmlinkage long sys32_rt_sigreturn(void)
>>>>
>>>>  set_current_blocked();
>>>>
>>>> -if (ia32_restore_sigcontext(regs, >uc.uc_mcontext))
>>>> +if (ia32_restore_sigcontext(regs, >uc.uc_mcontext, 
>>>> _cookie))
>>>> +goto badframe;
>>>> +
>>>> +if (user_cookie == NULL)
>>>> +user_cookie = (void __user *)((regs->sp - 4) + 
>&

Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 02/06/2016 11:35 PM, Mika Penttilä wrote:
> Hi,
> 
> 
> On 07.02.2016 01:39, Scott Bauer wrote:
>> This patch adds SROP mitigation logic to the x86 signal delivery
>> and sigreturn code. The cookie is placed in the unused alignment
>> space above the saved FP state, if it exists. If there is no FP
>> state to save then the cookie is placed in the alignment space above
>> the sigframe.
>>
>> Cc: Abhiram Balasubramanian 
>> Signed-off-by: Scott Bauer 
>> ---
>>  arch/x86/ia32/ia32_signal.c| 63 +---
>>  arch/x86/include/asm/fpu/signal.h  |  1 +
>>  arch/x86/include/asm/sighandling.h |  5 ++-
>>  arch/x86/kernel/fpu/signal.c   | 10 +
>>  arch/x86/kernel/signal.c   | 86 
>> +-
>>  5 files changed, 146 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>> index 0552884..2751f47 100644
>> --- a/arch/x86/ia32/ia32_signal.c
>> +++ b/arch/x86/ia32/ia32_signal.c
>> @@ -68,7 +68,8 @@
>>  }
>>  
>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>> -   struct sigcontext_32 __user *sc)
>> +   struct sigcontext_32 __user *sc,
>> +   void __user **user_cookie)
>>  {
>>  unsigned int tmpflags, err = 0;
>>  void __user *buf;
>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>>  buf = compat_ptr(tmp);
>>  } get_user_catch(err);
>>  
>> +/*
>> + * If there is fp state get cookie from the top of the fp state,
>> + * else get it from the top of the sig frame.
>> + */
>> +
>> +if (tmp != 0)
>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>> +else
>> +*user_cookie = NULL;
>> +
>>  err |= fpu__restore_sig(buf, 1);
>>  
>>  force_iret();
>> @@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
>>  struct pt_regs *regs = current_pt_regs();
>>  struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user 
>> *)(regs->sp-8);
>>  sigset_t set;
>> +void __user *user_cookie;
>>  
>>  if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>  goto badframe;
>> @@ -129,8 +141,15 @@ asmlinkage long sys32_sigreturn(void)
>>  
>>  set_current_blocked();
>>  
>> -if (ia32_restore_sigcontext(regs, >sc))
>> +if (ia32_restore_sigcontext(regs, >sc, _cookie))
>> +goto badframe;
>> +
>> +if (user_cookie == NULL)
>> +user_cookie = compat_ptr((regs->sp - 8) + sizeof(*frame));
>> +
>> +if (verify_clear_sigcookie(user_cookie))
>>  goto badframe;
>> +
>>  return regs->ax;
>>  
>>  badframe:
>> @@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
>>  {
>>  struct pt_regs *regs = current_pt_regs();
>>  struct rt_sigframe_ia32 __user *frame;
>> +void __user *user_cookie;
>>  sigset_t set;
>>  
>>  frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
>> @@ -153,7 +173,13 @@ asmlinkage long sys32_rt_sigreturn(void)
>>  
>>  set_current_blocked();
>>  
>> -if (ia32_restore_sigcontext(regs, >uc.uc_mcontext))
>> +if (ia32_restore_sigcontext(regs, >uc.uc_mcontext, _cookie))
>> +goto badframe;
>> +
>> +if (user_cookie == NULL)
>> +user_cookie = (void __user *)((regs->sp - 4) + sizeof(*frame));
> regs->sp is already restored so you should use frame instead.
> 
> --Mika
> 

Nice catch, thank you. I'm surprised I haven't hit this issue yet.
I've been running these patches for 3 weeks on 2 different machines and
haven't had an issue. I'll submit v3 with this fix a bit later, I want
to see if anyone else has stuff to fix.

Thanks again

Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies

[Scotty Bauer]

On 02/06/2016 11:35 PM, Mika Penttilä wrote:
> Hi,
> 
> 
> On 07.02.2016 01:39, Scott Bauer wrote:
>> This patch adds SROP mitigation logic to the x86 signal delivery
>> and sigreturn code. The cookie is placed in the unused alignment
>> space above the saved FP state, if it exists. If there is no FP
>> state to save then the cookie is placed in the alignment space above
>> the sigframe.
>>
>> Cc: Abhiram Balasubramanian 
>> Signed-off-by: Scott Bauer 
>> ---
>>  arch/x86/ia32/ia32_signal.c| 63 +---
>>  arch/x86/include/asm/fpu/signal.h  |  1 +
>>  arch/x86/include/asm/sighandling.h |  5 ++-
>>  arch/x86/kernel/fpu/signal.c   | 10 +
>>  arch/x86/kernel/signal.c   | 86 
>> +-
>>  5 files changed, 146 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>> index 0552884..2751f47 100644
>> --- a/arch/x86/ia32/ia32_signal.c
>> +++ b/arch/x86/ia32/ia32_signal.c
>> @@ -68,7 +68,8 @@
>>  }
>>  
>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
>> -   struct sigcontext_32 __user *sc)
>> +   struct sigcontext_32 __user *sc,
>> +   void __user **user_cookie)
>>  {
>>  unsigned int tmpflags, err = 0;
>>  void __user *buf;
>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>>  buf = compat_ptr(tmp);
>>  } get_user_catch(err);
>>  
>> +/*
>> + * If there is fp state get cookie from the top of the fp state,
>> + * else get it from the top of the sig frame.
>> + */
>> +
>> +if (tmp != 0)
>> +*user_cookie = compat_ptr(tmp + fpu__getsize(1));
>> +else
>> +*user_cookie = NULL;
>> +
>>  err |= fpu__restore_sig(buf, 1);
>>  
>>  force_iret();
>> @@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
>>  struct pt_regs *regs = current_pt_regs();
>>  struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user 
>> *)(regs->sp-8);
>>  sigset_t set;
>> +void __user *user_cookie;
>>  
>>  if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>  goto badframe;
>> @@ -129,8 +141,15 @@ asmlinkage long sys32_sigreturn(void)
>>  
>>  set_current_blocked();
>>  
>> -if (ia32_restore_sigcontext(regs, >sc))
>> +if (ia32_restore_sigcontext(regs, >sc, _cookie))
>> +goto badframe;
>> +
>> +if (user_cookie == NULL)
>> +user_cookie = compat_ptr((regs->sp - 8) + sizeof(*frame));
>> +
>> +if (verify_clear_sigcookie(user_cookie))
>>  goto badframe;
>> +
>>  return regs->ax;
>>  
>>  badframe:
>> @@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
>>  {
>>  struct pt_regs *regs = current_pt_regs();
>>  struct rt_sigframe_ia32 __user *frame;
>> +void __user *user_cookie;
>>  sigset_t set;
>>  
>>  frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
>> @@ -153,7 +173,13 @@ asmlinkage long sys32_rt_sigreturn(void)
>>  
>>  set_current_blocked();
>>  
>> -if (ia32_restore_sigcontext(regs, >uc.uc_mcontext))
>> +if (ia32_restore_sigcontext(regs, >uc.uc_mcontext, _cookie))
>> +goto badframe;
>> +
>> +if (user_cookie == NULL)
>> +user_cookie = (void __user *)((regs->sp - 4) + sizeof(*frame));
> regs->sp is already restored so you should use frame instead.
> 
> --Mika
> 

Nice catch, thank you. I'm surprised I haven't hit this issue yet.
I've been running these patches for 3 weeks on 2 different machines and
haven't had an issue. I'll submit v3 with this fix a bit later, I want
to see if anyone else has stuff to fix.

Thanks again

Re: dm ioctl: Access user-land memory through safe functions.

[Scotty Bauer]

On 01/05/2016 02:13 PM, Mike Snitzer wrote:
> On Tue, Jan 05 2016 at  3:16pm -0500,
> Mike Snitzer  wrote:
> 
>> On Tue, Dec 08 2015 at  1:26pm -0500,
>> Scotty Bauer  wrote:
>>
>>> Friendly ping, is anyone interested in this?
>>
>> The passed @user argument is flagged via __user so it can be
>> deferenced directly.  It does look like directly deferencing
>> user->version is wrong.
>>
>> But even if such indirect access is needed (because __user flag is only
>> applicable to @user arg, not the contained version member) we could more
>> easily just do something like this no?:
>>
>>   uint32_t __user *versionp = (uint32_t __user *)user->version;
>>   ...
>>   if (copy_from_user(version, versionp, sizeof(version)))
>>  return -EFAULT;
>>
>> I've staged the following, thanks:
>> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5=bffc9e237a0c3176712bcd93fc6a184a61e0df26
> 
> Alasdair helped me understand that we do need your original fix.
> I've staged it for 4.5 (and stable@) here:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5=ead3db62bf10fe143bec99e7b7ff370d7a6d23ef
> 
> Thanks again,
> Mike
> --

This broke linux-next because I'm dumb and didn't test it. I thought it was a 
trivial enough of a patch that I wouldn't screw it up, but I did.

I incorrectly assumed that user->version was essentially a pointer in userland, 
not a flat chunk of memory. Ie it was a pointer to some malloc'd region, not an 
inlined version[3].

I thought it was this:
struct dm_ioctl {

uint32_t *version;
...
}

It is really this:

struct dm_ioctl {

uint32_t version[3];

}

I was trying to get the values out of *version, which would have been a 
pointer, but instead what the code ended up doing was actually getting 8 bytes 
of the version (think 4,3,1) out and trying to access that version as a memory 
address, oops.

It turns out that the original code is correct and doesn't actually touch user 
memory without a copy_from_user().  Gcc is smart enough to see that version[3] 
is inlined, and it can emit code which simply takes the userland pointer 
(struct dm_ioctl __user user), and calculates on offset based on the pointer, 
thus no actual user dereference occurs. Had the struct looked like the first 
example I believe the patch would work.

I'm wondering now if we should switch the code a bit to make it less ambiguous, 
so someone like me doesn't come along again thinking the code dereferences 
userland memory and waste everyones time.

I've attached a patch based off linux-next-20150616 which reverts my broken 
code but adds an & to the front of user->version so it looks like the code is 
doing the right thing.

If I should be basing my patch off something other than linux-next let me know 
and I'll rewrite it, or we can just revert the old patch and ignore this one.

Thanks and very sorry for the confusion and breakage.



>From 7dde54b74e4543b6f03ceb57f9479a1d402a3fd1 Mon Sep 17 00:00:00 2001
From: Scotty Bauer 
Date: Wed, 6 Jan 2016 18:17:35 -0700
Subject: [PATCH] dm ioctl: disambiguate the user pointer calculation

This patch adds an & in front of user->version, in hopes of making
it clear that user-memory is not being touched.

Signed-off-by: Scotty Bauer 
---
 drivers/md/dm-ioctl.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index fa5bf54..81190df 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1642,13 +1642,9 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
-	uint32_t __user *versionp;
 	int r = 0;
 
-	if (copy_from_user(, >version, sizeof(versionp)))
-		return -EFAULT;
-
-	if (copy_from_user(version, versionp, sizeof(version)))
+	if (copy_from_user(version, >version, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||
@@ -1667,7 +1663,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(versionp, version, sizeof(version)))
+	if (copy_to_user(>version, version, sizeof(version)))
 		return -EFAULT;
 
 	return r;
-- 
1.9.1

Re: dm ioctl: Access user-land memory through safe functions.

[Scotty Bauer]

On 01/05/2016 02:13 PM, Mike Snitzer wrote:
> On Tue, Jan 05 2016 at  3:16pm -0500,
> Mike Snitzer <snit...@redhat.com> wrote:
> 
>> On Tue, Dec 08 2015 at  1:26pm -0500,
>> Scotty Bauer <sba...@eng.utah.edu> wrote:
>>
>>> Friendly ping, is anyone interested in this?
>>
>> The passed @user argument is flagged via __user so it can be
>> deferenced directly.  It does look like directly deferencing
>> user->version is wrong.
>>
>> But even if such indirect access is needed (because __user flag is only
>> applicable to @user arg, not the contained version member) we could more
>> easily just do something like this no?:
>>
>>   uint32_t __user *versionp = (uint32_t __user *)user->version;
>>   ...
>>   if (copy_from_user(version, versionp, sizeof(version)))
>>  return -EFAULT;
>>
>> I've staged the following, thanks:
>> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5=bffc9e237a0c3176712bcd93fc6a184a61e0df26
> 
> Alasdair helped me understand that we do need your original fix.
> I've staged it for 4.5 (and stable@) here:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5=ead3db62bf10fe143bec99e7b7ff370d7a6d23ef
> 
> Thanks again,
> Mike
> --

This broke linux-next because I'm dumb and didn't test it. I thought it was a 
trivial enough of a patch that I wouldn't screw it up, but I did.

I incorrectly assumed that user->version was essentially a pointer in userland, 
not a flat chunk of memory. Ie it was a pointer to some malloc'd region, not an 
inlined version[3].

I thought it was this:
struct dm_ioctl {

uint32_t *version;
...
}

It is really this:

struct dm_ioctl {

uint32_t version[3];

}

I was trying to get the values out of *version, which would have been a 
pointer, but instead what the code ended up doing was actually getting 8 bytes 
of the version (think 4,3,1) out and trying to access that version as a memory 
address, oops.

It turns out that the original code is correct and doesn't actually touch user 
memory without a copy_from_user().  Gcc is smart enough to see that version[3] 
is inlined, and it can emit code which simply takes the userland pointer 
(struct dm_ioctl __user user), and calculates on offset based on the pointer, 
thus no actual user dereference occurs. Had the struct looked like the first 
example I believe the patch would work.

I'm wondering now if we should switch the code a bit to make it less ambiguous, 
so someone like me doesn't come along again thinking the code dereferences 
userland memory and waste everyones time.

I've attached a patch based off linux-next-20150616 which reverts my broken 
code but adds an & to the front of user->version so it looks like the code is 
doing the right thing.

If I should be basing my patch off something other than linux-next let me know 
and I'll rewrite it, or we can just revert the old patch and ignore this one.

Thanks and very sorry for the confusion and breakage.



>From 7dde54b74e4543b6f03ceb57f9479a1d402a3fd1 Mon Sep 17 00:00:00 2001
From: Scotty Bauer <sba...@eng.utah.edu>
Date: Wed, 6 Jan 2016 18:17:35 -0700
Subject: [PATCH] dm ioctl: disambiguate the user pointer calculation

This patch adds an & in front of user->version, in hopes of making
it clear that user-memory is not being touched.

Signed-off-by: Scotty Bauer <sba...@eng.utah.edu>
---
 drivers/md/dm-ioctl.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index fa5bf54..81190df 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1642,13 +1642,9 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
-	uint32_t __user *versionp;
 	int r = 0;
 
-	if (copy_from_user(, >version, sizeof(versionp)))
-		return -EFAULT;
-
-	if (copy_from_user(version, versionp, sizeof(version)))
+	if (copy_from_user(version, >version, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||
@@ -1667,7 +1663,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(versionp, version, sizeof(version)))
+	if (copy_to_user(>version, version, sizeof(version)))
 		return -EFAULT;
 
 	return r;
-- 
1.9.1

Re: [PATCH] dm ioctl: Access user-land memory through safe functions.

[Scotty Bauer]

On 12/01/2015 11:11 AM, Scotty wrote:
> 
> 0001-dm-ioctl-Access-user-land-memory-through-safe-functi.patch
> 
> 
> From b26adf880eba03ac6f2b1dd87426bb96fd2a0282 Mon Sep 17 00:00:00 2001
> From: Scotty Bauer 
> Date: Tue, 1 Dec 2015 10:52:46 -0700
> Subject: [PATCH] dm ioctl: Access user-land memory through safe functions.
> 
> This patch fixes a user-land dereference. Now we use
> the safe copy_from_user to access the memory.
> 
> Signed-off-by: Scotty Bauer 
> ---
>  drivers/md/dm-ioctl.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 80a4395..39a9d1a 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1642,9 +1642,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int 
> *ioctl_flags)
>  static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
>  {
>   uint32_t version[3];
> + uint32_t __user *version_ptr;
>   int r = 0;
>  
> - if (copy_from_user(version, user->version, sizeof(version)))
> + if (copy_from_user(_ptr, >version, sizeof(version_ptr)))
> + return -EFAULT;
> +
> + if (copy_from_user(version, version_ptr, sizeof(version)))
>   return -EFAULT;
>  
>   if ((DM_VERSION_MAJOR != version[0]) ||
> @@ -1663,7 +1667,7 @@ static int check_version(unsigned int cmd, struct 
> dm_ioctl __user *user)
>   version[0] = DM_VERSION_MAJOR;
>   version[1] = DM_VERSION_MINOR;
>   version[2] = DM_VERSION_PATCHLEVEL;
> - if (copy_to_user(user->version, version, sizeof(version)))
> + if (copy_to_user(version_ptr, version, sizeof(version)))
>   return -EFAULT;
>  
>   return r;
> -- 


Friendly ping, is anyone interested in this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dm ioctl: Access user-land memory through safe functions.

[Scotty Bauer]

On 12/01/2015 11:11 AM, Scotty wrote:
> 
> 0001-dm-ioctl-Access-user-land-memory-through-safe-functi.patch
> 
> 
> From b26adf880eba03ac6f2b1dd87426bb96fd2a0282 Mon Sep 17 00:00:00 2001
> From: Scotty Bauer <sba...@eng.utah.edu>
> Date: Tue, 1 Dec 2015 10:52:46 -0700
> Subject: [PATCH] dm ioctl: Access user-land memory through safe functions.
> 
> This patch fixes a user-land dereference. Now we use
> the safe copy_from_user to access the memory.
> 
> Signed-off-by: Scotty Bauer <sba...@eng.utah.edu>
> ---
>  drivers/md/dm-ioctl.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 80a4395..39a9d1a 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1642,9 +1642,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int 
> *ioctl_flags)
>  static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
>  {
>   uint32_t version[3];
> + uint32_t __user *version_ptr;
>   int r = 0;
>  
> - if (copy_from_user(version, user->version, sizeof(version)))
> + if (copy_from_user(_ptr, >version, sizeof(version_ptr)))
> + return -EFAULT;
> +
> + if (copy_from_user(version, version_ptr, sizeof(version)))
>   return -EFAULT;
>  
>   if ((DM_VERSION_MAJOR != version[0]) ||
> @@ -1663,7 +1667,7 @@ static int check_version(unsigned int cmd, struct 
> dm_ioctl __user *user)
>   version[0] = DM_VERSION_MAJOR;
>   version[1] = DM_VERSION_MINOR;
>   version[2] = DM_VERSION_PATCHLEVEL;
> - if (copy_to_user(user->version, version, sizeof(version)))
> + if (copy_to_user(version_ptr, version, sizeof(version)))
>   return -EFAULT;
>  
>   return r;
> -- 


Friendly ping, is anyone interested in this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Accessing user-land memory without safe functions

[Scotty Bauer]

Hi Jon,

Thanks for the response.

Here is one such example, although benign due to the
function being called with CAP_SYS_ADMIN. I'll work on patches for some of the
other issues and send those soon as well.

https://lkml.org/lkml/2015/12/1/566




On 12/03/2015 05:12 PM, Jonathan Corbet wrote:
> On Thu, 3 Dec 2015 13:54:30 -0700
> Scotty Bauer  wrote:
> 
>> Since I've seen this a couple times now I'm wondering if my
>> understanding of touching user-land memory is flawed.
>>
>> For the above example Ioctl, the proper way to get access to those fields
>> through the safe copy_from_user or get_user() functions, correct?
>>
>> I'm wondering if I should submit patches to fix the issues I've found,
>> but now I'm doubting whether they're really issues at all.
> 
> They sound like bugs to me, though it would be easier to say for sure with
> a pointer to a specific function in the kernel source.  Please point
> something out, or, perhaps better, send a patch fixing one of them.
> 
> Thanks,
> 
> jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Accessing user-land memory without safe functions

[Scotty Bauer]

I Have been auditing a few drivers and have found some of them are
accessing user-land memory without either mapping the pages in, or
copying the data via the safe user access apis. 

The thing I have mostly been seeing is something along the lines of:

ioctl(etc, etc, arg) {

char buf[32];
__user *some_struct  = (type cast) arg;
 
size_t amount = some_struct->amount;

** do size check on amount **

copy_from_user(buf, some_struct->some_uland_addr, amount);

}


Above you see 2 unsafe user-land dereferences, the
some_struct->amount and some_struct->some_uland_addr.


Since I've seen this a couple times now I'm wondering if my
understanding of touching user-land memory is flawed.

For the above example Ioctl, the proper way to get access to those fields
through the safe copy_from_user or get_user() functions, correct?

I'm wondering if I should submit patches to fix the issues I've found,
but now I'm doubting whether they're really issues at all.

Thanks,
Scotty
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Accessing user-land memory without safe functions

[Scotty Bauer]

I Have been auditing a few drivers and have found some of them are
accessing user-land memory without either mapping the pages in, or
copying the data via the safe user access apis. 

The thing I have mostly been seeing is something along the lines of:

ioctl(etc, etc, arg) {

char buf[32];
__user *some_struct  = (type cast) arg;
 
size_t amount = some_struct->amount;

** do size check on amount **

copy_from_user(buf, some_struct->some_uland_addr, amount);

}


Above you see 2 unsafe user-land dereferences, the
some_struct->amount and some_struct->some_uland_addr.


Since I've seen this a couple times now I'm wondering if my
understanding of touching user-land memory is flawed.

For the above example Ioctl, the proper way to get access to those fields
through the safe copy_from_user or get_user() functions, correct?

I'm wondering if I should submit patches to fix the issues I've found,
but now I'm doubting whether they're really issues at all.

Thanks,
Scotty
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Accessing user-land memory without safe functions

[Scotty Bauer]

Hi Jon,

Thanks for the response.

Here is one such example, although benign due to the
function being called with CAP_SYS_ADMIN. I'll work on patches for some of the
other issues and send those soon as well.

https://lkml.org/lkml/2015/12/1/566




On 12/03/2015 05:12 PM, Jonathan Corbet wrote:
> On Thu, 3 Dec 2015 13:54:30 -0700
> Scotty Bauer <sba...@eng.utah.edu> wrote:
> 
>> Since I've seen this a couple times now I'm wondering if my
>> understanding of touching user-land memory is flawed.
>>
>> For the above example Ioctl, the proper way to get access to those fields
>> through the safe copy_from_user or get_user() functions, correct?
>>
>> I'm wondering if I should submit patches to fix the issues I've found,
>> but now I'm doubting whether they're really issues at all.
> 
> They sound like bugs to me, though it would be easier to say for sure with
> a pointer to a specific function in the kernel source.  Please point
> something out, or, perhaps better, send a patch fixing one of them.
> 
> Thanks,
> 
> jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ftrace] possible to implement user-space tracers?

[Scotty Bauer]

It is possible to trace from userland, Android does it.

Essentially you need to write your data into 
/sys/kernel/debug/tracing/trace_marker


then read it out of /sys/kernel/debug/tracing/trace


If you care how the implementation works you can read it in 
/kernel/tracing/trace.c 
(http://lxr.free-electrons.com/source/kernel/trace/trace.c) search for 
tracing_mark_fops and tracing_fops.



In Android here are the relevant files you will need to see how it's 
used in userland:

Atrace (for reading data out + setting things up):
https://android.googlesource.com/platform/frameworks/native/+/master/cmds/atrace/atrace.cpp
https://android.googlesource.com/platform/external/chromium-trace/+/master 
(host side setup of the device)


trace-dev (for writing data in):
https://android.googlesource.com/platform/system/core/+/master/libcutils/trace-dev.c

Once we have all the data we usually run it through trace-viewer, 
(https://github.com/catapult-project/catapult)


Cheers.



On 2015-08-11 06:53, Kun Huang wrote:

Hi ftrace developers

I'm developing a huge python based programs and facing performance
issue everyday. I like the ftrace system and hope there could be a
tracer to trace my python codes. Is it possible or is it

worthy

to do this?

---

Kun
--
To unsubscribe from this list: send the line "unsubscribe

linux-kernel" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[6]

Please read the FAQ at  http://www.tux.org/lkml/ [7]


Links:
--
[1] http://lxr.free-electrons.com/source/kernel/trace/trace.c
[2]
https://android.googlesource.com/platform/frameworks/native/+/master/cmds/atrace/atrace.cpp
[3] 
https://android.googlesource.com/platform/external/chromium-trace/+/master

[4]
https://android.googlesource.com/platform/system/core/+/master/libcutils/trace-dev.c
[5] https://github.com/catapult-project/catapult
[6] http://vger.kernel.org/majordomo-info.html
[7] http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ftrace] possible to implement user-space tracers?

[Scotty Bauer]

It is possible to trace from userland, Android does it.

Essentially you need to write your data into 
/sys/kernel/debug/tracing/trace_marker


then read it out of /sys/kernel/debug/tracing/trace


If you care how the implementation works you can read it in 
/kernel/tracing/trace.c 
(http://lxr.free-electrons.com/source/kernel/trace/trace.c) search for 
tracing_mark_fops and tracing_fops.



In Android here are the relevant files you will need to see how it's 
used in userland:

Atrace (for reading data out + setting things up):
https://android.googlesource.com/platform/frameworks/native/+/master/cmds/atrace/atrace.cpp
https://android.googlesource.com/platform/external/chromium-trace/+/master 
(host side setup of the device)


trace-dev (for writing data in):
https://android.googlesource.com/platform/system/core/+/master/libcutils/trace-dev.c

Once we have all the data we usually run it through trace-viewer, 
(https://github.com/catapult-project/catapult)


Cheers.



On 2015-08-11 06:53, Kun Huang wrote:

Hi ftrace developers

I'm developing a huge python based programs and facing performance
issue everyday. I like the ftrace system and hope there could be a
tracer to tracereport my python codes. Is it possible or is it

worthy

to do this?

---

Kun
--
To unsubscribe from this list: send the line unsubscribe

linux-kernel in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[6]

Please read the FAQ at  http://www.tux.org/lkml/ [7]


Links:
--
[1] http://lxr.free-electrons.com/source/kernel/trace/trace.c
[2]
https://android.googlesource.com/platform/frameworks/native/+/master/cmds/atrace/atrace.cpp
[3] 
https://android.googlesource.com/platform/external/chromium-trace/+/master

[4]
https://android.googlesource.com/platform/system/core/+/master/libcutils/trace-dev.c
[5] https://github.com/catapult-project/catapult
[6] http://vger.kernel.org/majordomo-info.html
[7] http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

On 02/11/2015 02:55 PM, H. Peter Anvin wrote:
> On 01/30/2015 01:26 PM, Scotty Bauer wrote:
>> mwait_play_dead previously issued a CLFLUSH to work around a bug on
>> some xeon processors. We can now determine if the CPU is a buggy CPU.
>> This patch checks if if we're on a buggy CPU which allows non-buggy
>> cpu's to eliminate the CLFLUSH.
> Here is my first question: does this matter at all?  Otherwise I don't
> see a point.
>
>   -hpa
>
>
Do you get the same effect? Sure, but is the previous way the right way to do 
it? In my opinion no, but I'm not the one merging code its up to someone more 
experienced to determine if the change is warranted. The change is slightly 
faster on non-buggy cpu, but like you mention, is that relevant when the 
machine is going into idle?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

[Scotty Bauer]

For what its worth I tried it out and it works fine on my end.

Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the 
alternatives.

I think it may be worth doing a patch that is almost verbatim to this for 
mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things 
consistent. I can work on that over the weekend.

--Scotty

On 02/06/2015 09:13 AM, Borislav Petkov wrote:
> From: Borislav Petkov 
> Subject: [PATCH] x86, smpboot: Call CLFLUSH only on 
> X86_BUG_CLFLUSH_MONITOR-affected CPUs
>
> Make the AAI65 erratum workaround for Xeon 7400 machines only instead of
> punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.
>
> Based on a patch originally by Scotty Bauer .
>
> Cc: Scotty Bauer 
> Signed-off-by: Borislav Petkov 
> ---
>  arch/x86/kernel/smpboot.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 6d7022c683e3..771ebd6e8b77 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
>* case where we return around the loop.
>*/
>   mb();
> - clflush(mwait_ptr);
> +
> + asm volatile(ALTERNATIVE("", "clflush %[p]",
> +  X86_BUG_CLFLUSH_MONITOR)
> + : [p] "+m" (*(unsigned long *)mwait_ptr));
> +
>   mb();
>   __monitor(mwait_ptr, 0, 0);
>   mb();

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

[Scotty Bauer]

For what its worth I tried it out and it works fine on my end.

Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the 
alternatives.

I think it may be worth doing a patch that is almost verbatim to this for 
mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things 
consistent. I can work on that over the weekend.

--Scotty

On 02/06/2015 09:13 AM, Borislav Petkov wrote:
 From: Borislav Petkov b...@suse.de
 Subject: [PATCH] x86, smpboot: Call CLFLUSH only on 
 X86_BUG_CLFLUSH_MONITOR-affected CPUs

 Make the AAI65 erratum workaround for Xeon 7400 machines only instead of
 punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.

 Based on a patch originally by Scotty Bauer sba...@eng.utah.edu.

 Cc: Scotty Bauer sba...@eng.utah.edu
 Signed-off-by: Borislav Petkov b...@suse.de
 ---
  arch/x86/kernel/smpboot.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

 diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
 index 6d7022c683e3..771ebd6e8b77 100644
 --- a/arch/x86/kernel/smpboot.c
 +++ b/arch/x86/kernel/smpboot.c
 @@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
* case where we return around the loop.
*/
   mb();
 - clflush(mwait_ptr);
 +
 + asm volatile(ALTERNATIVE(, clflush %[p],
 +  X86_BUG_CLFLUSH_MONITOR)
 + : [p] +m (*(unsigned long *)mwait_ptr));
 +
   mb();
   __monitor(mwait_ptr, 0, 0);
   mb();

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

On 02/11/2015 02:55 PM, H. Peter Anvin wrote:
 On 01/30/2015 01:26 PM, Scotty Bauer wrote:
 mwait_play_dead previously issued a CLFLUSH to work around a bug on
 some xeon processors. We can now determine if the CPU is a buggy CPU.
 This patch checks if if we're on a buggy CPU which allows non-buggy
 cpu's to eliminate the CLFLUSH.
 Here is my first question: does this matter at all?  Otherwise I don't
 see a point.

   -hpa


Do you get the same effect? Sure, but is the previous way the right way to do 
it? In my opinion no, but I'm not the one merging code its up to someone more 
experienced to determine if the change is warranted. The change is slightly 
faster on non-buggy cpu, but like you mention, is that relevant when the 
machine is going into idle?


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon 
processors. We can now determine if the CPU is a buggy CPU. This patch checks 
if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the 
CLFLUSH.





>From 3da1be5c998a8d51f98fdba09b3cb477526c5ff3 Mon Sep 17 00:00:00 2001
From: Scott Bauer 
Date: Fri, 30 Jan 2015 14:10:37 -0700
Subject: [PATCH] Add check to determine if CLFLUSH is actually necessary
 before monitor/wait

Signed-off-by: Scott Bauer 
---
 arch/x86/kernel/smpboot.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c..552ff48 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1384,6 +1384,7 @@ static inline void mwait_play_dead(void)
 	unsigned int highest_subcstate = 0;
 	void *mwait_ptr;
 	int i;
+	int cpu;
 
 	if (!this_cpu_has(X86_FEATURE_MWAIT))
 		return;
@@ -1420,6 +1421,7 @@ static inline void mwait_play_dead(void)
 	 * content is immaterial as it is not actually modified in any way.
 	 */
 	mwait_ptr = _thread_info()->flags;
+	cpu = smp_processor_id();
 
 	wbinvd();
 
@@ -1430,10 +1432,15 @@ static inline void mwait_play_dead(void)
 		 * needed, but it should be harmless in either case.
 		 * The WBINVD is insufficient due to the spurious-wakeup
 		 * case where we return around the loop.
+		 *
+		 * Check if the CLFLUSH is actually necessary before calling
 		 */
-		mb();
-		clflush(mwait_ptr);
-		mb();
+		if (cpu_has_bug(_data(cpu), X86_BUG_CLFLUSH_MONITOR)) {
+			mb();
+			clflush(mwait_ptr);
+			mb();
+		}
+
 		__monitor(mwait_ptr, 0, 0);
 		mb();
 		__mwait(eax, 0);
-- 
1.9.1

[PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon 
processors. We can now determine if the CPU is a buggy CPU. This patch checks 
if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the 
CLFLUSH.





From 3da1be5c998a8d51f98fdba09b3cb477526c5ff3 Mon Sep 17 00:00:00 2001
From: Scott Bauer sba...@eng.utah.edu
Date: Fri, 30 Jan 2015 14:10:37 -0700
Subject: [PATCH] Add check to determine if CLFLUSH is actually necessary
 before monitor/wait

Signed-off-by: Scott Bauer sba...@eng.utah.edu
---
 arch/x86/kernel/smpboot.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c..552ff48 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1384,6 +1384,7 @@ static inline void mwait_play_dead(void)
 	unsigned int highest_subcstate = 0;
 	void *mwait_ptr;
 	int i;
+	int cpu;
 
 	if (!this_cpu_has(X86_FEATURE_MWAIT))
 		return;
@@ -1420,6 +1421,7 @@ static inline void mwait_play_dead(void)
 	 * content is immaterial as it is not actually modified in any way.
 	 */
 	mwait_ptr = current_thread_info()-flags;
+	cpu = smp_processor_id();
 
 	wbinvd();
 
@@ -1430,10 +1432,15 @@ static inline void mwait_play_dead(void)
 		 * needed, but it should be harmless in either case.
 		 * The WBINVD is insufficient due to the spurious-wakeup
 		 * case where we return around the loop.
+		 *
+		 * Check if the CLFLUSH is actually necessary before calling
 		 */
-		mb();
-		clflush(mwait_ptr);
-		mb();
+		if (cpu_has_bug(cpu_data(cpu), X86_BUG_CLFLUSH_MONITOR)) {
+			mb();
+			clflush(mwait_ptr);
+			mb();
+		}
+
 		__monitor(mwait_ptr, 0, 0);
 		mb();
 		__mwait(eax, 0);
-- 
1.9.1

Re: [PATCH] kern/sys: Compat sysinfo syscall fix undefined behavior

[Scotty Bauer]

On 09/04/2014 02:14 PM, Andrew Morton wrote:
> If I'm reading it correctly, this is all dead code because si_meminfo() 
> unconditionally sets sysinfo.mem_unit to PAGE_SIZE. It could all do with a 
> bit of a cleanup, I suspect. 
I'll do a little more research on this and do further clean up, if required.

-Scotty
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] kern/sys: Compat sysinfo syscall fix undefined behavior

[Scotty Bauer]

Fix undefined behavior and compiler warning by replacing right
shift 32 with upper_32_bits macro

Signed-off-by: Scotty Bauer 
---
 kernel/sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index ce81291..c78530b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2135,7 +2135,7 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
 	/* Check to see if any memory value is too large for 32-bit and scale
 	 *  down if needed
 	 */
-	if ((s.totalram >> 32) || (s.totalswap >> 32)) {
+	if (upper_32_bits(s.totalram) || upper_32_bits(s.totalswap)) {
 		int bitcount = 0;
 
 		while (s.mem_unit < PAGE_SIZE) {
-- 
1.8.3.2

[PATCH] kern/sys: Compat sysinfo syscall fix undefined behavior

[Scotty Bauer]

Fix undefined behavior and compiler warning by replacing right
shift 32 with upper_32_bits macro

Signed-off-by: Scotty Bauer sba...@eng.utah.edu
---
 kernel/sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index ce81291..c78530b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2135,7 +2135,7 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
 	/* Check to see if any memory value is too large for 32-bit and scale
 	 *  down if needed
 	 */
-	if ((s.totalram  32) || (s.totalswap  32)) {
+	if (upper_32_bits(s.totalram) || upper_32_bits(s.totalswap)) {
 		int bitcount = 0;
 
 		while (s.mem_unit  PAGE_SIZE) {
-- 
1.8.3.2

Re: [PATCH] kern/sys: Compat sysinfo syscall fix undefined behavior

[Scotty Bauer]

On 09/04/2014 02:14 PM, Andrew Morton wrote:
 If I'm reading it correctly, this is all dead code because si_meminfo() 
 unconditionally sets sysinfo.mem_unit to PAGE_SIZE. It could all do with a 
 bit of a cleanup, I suspect. 
I'll do a little more research on this and do further clean up, if required.

-Scotty
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Compat sysinfo syscall (kernel/sys.c) relying on undefined behavior?

[Scotty Bauer]

 am getting acquainted with the linux kernel and to do so I've been browsing 
the source.


In the compat version of sysinfo, kernel/sys.c we see the following:

COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
{
struct sysinfo s;

do_sysinfo();

/* Check to see if any memory value is too large for 32-bit and scale
 *  down if needed
 */
if ((s.totalram >> 32) || (s.totalswap >> 32)) {
int bitcount = 0;

...


s.totalram is a u32, and the standard says:
"If the value of the right operand is negative or is greater than or equal to 
the width 
of the promoted left operand, the behavior is undefined."

Is there some promotion, compiler flag, something obvious that I am missing, or 
is this a 
problem?


Best,
Scotty

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Compat sysinfo syscall (kernel/sys.c) relying on undefined behavior?

[Scotty Bauer]

 am getting acquainted with the linux kernel and to do so I've been browsing 
the source.


In the compat version of sysinfo, kernel/sys.c we see the following:

COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
{
struct sysinfo s;

do_sysinfo(s);

/* Check to see if any memory value is too large for 32-bit and scale
 *  down if needed
 */
if ((s.totalram  32) || (s.totalswap  32)) {
int bitcount = 0;

...


s.totalram is a u32, and the standard says:
If the value of the right operand is negative or is greater than or equal to 
the width 
of the promoted left operand, the behavior is undefined.

Is there some promotion, compiler flag, something obvious that I am missing, or 
is this a 
problem?


Best,
Scotty

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/