Message ID | 1236145172.189228.314429009881.1.gpush@pingu |
---|---|
State | Accepted |
Delegated to: | Jeremy Kerr |
Headers | show |
On Wed, 4 Mar 2009, Jeremy Kerr wrote: > We need to offset by *pos bytes, not *pos words. > > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > > --- > arch/powerpc/platforms/cell/spufs/file.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c > index 83ef889..6b10877 100644 > --- a/arch/powerpc/platforms/cell/spufs/file.c > +++ b/arch/powerpc/platforms/cell/spufs/file.c > @@ -578,7 +578,7 @@ spufs_regs_write(struct file *file, const char __user *buffer, > if (ret) > return ret; > > - ret = copy_from_user(lscsa->gprs + *pos - size, > + ret = copy_from_user((char *)lscsa->gprs + *pos - size, > buffer, size) ? -EFAULT : size; > > spu_release_saved(ctx); Could this be abused by an attacker to write registers or local store he's not allowed to do? Should it be backported to stable? With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010
Hi Geert, > Could this be abused by an attacker to write registers or local store > he's not allowed to do? It looks like the user can only overwrite fields that it already has access to. There's struct spu_lscsa: struct spu_lscsa { struct spu_reg128 gprs[128]; struct spu_reg128 fpcr; struct spu_reg128 decr; struct spu_reg128 decr_status; struct spu_reg128 ppu_mb; struct spu_reg128 ppuint_mb; struct spu_reg128 tag_mask; struct spu_reg128 event_mask; struct spu_reg128 srr0; struct spu_reg128 stopped_status; unsigned char ls[LS_SIZE] __attribute__((aligned(65536))); }; where spu_reg128 is a u32[4]. The maximum 'allowed' write offset to the regs file is 2047. The (incorrect) maximum offset calculated by the old code would be 8188 (2047 * 4) bytes into struct spu_lscsa. So, 8188 bytes covers all of the registers, but ends somewhere before the start of the ls area (within the ls alignment padding). Let's look at the registers: gprs: user-writable fpcr: user-writable decr: user-writable decr_status: only affects user-settable SPE state ppu_mb: only affects user-settable SPE state ppuint_mb: only affects user-settable SPE state tag_mask: only affects user-settable SPE state event_mask: only affects user-settable SPE state srr0: only affects user-settable SPE state stopped_status: only affects user-settable SPE state So, I think we're fine. All a user can do with this bug is mess up their own SPE state. > Should it be backported to stable? Yes, I'll submit to the stable tree too. Cheers, Jeremy
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c index 83ef889..6b10877 100644 --- a/arch/powerpc/platforms/cell/spufs/file.c +++ b/arch/powerpc/platforms/cell/spufs/file.c @@ -578,7 +578,7 @@ spufs_regs_write(struct file *file, const char __user *buffer, if (ret) return ret; - ret = copy_from_user(lscsa->gprs + *pos - size, + ret = copy_from_user((char *)lscsa->gprs + *pos - size, buffer, size) ? -EFAULT : size; spu_release_saved(ctx);
We need to offset by *pos bytes, not *pos words. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- arch/powerpc/platforms/cell/spufs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)