Buffer overflow in ZSNES since update to raring

Status changed to 'Confirmed' because the bug affects multiple users.

I am seeing the same bug in zsnes_1.510+bz2-5ubuntu2_i386.deb on the 32-bit architecture (3.8.0-19-generic #29-Ubuntu SMP i686 i686 i686 GNU/Linux)

Same bug on 13.04 64 bit. I actually thought it was a 64 bit only bug.

I'm also running raring on amd64. Downgraded to quantal (downloaded .debs individually, required both libao4 and libao-common as well as the zsnes deb) which works fine.

In quantal AMD64 it worked fine,but as soon as
i upgraded to raring,it didn't work.

Reproducible on 13.04 32 bit with latest updates here. Any update on this?

Same issue here running Mint15 - found this older deb that worked though: http://mirrors.kernel.org/ubuntu/pool/universe/z/zsnes/zsnes_1.510-2.2ubuntu4_i386.deb
Just be aware that your update manager will try and replace it with the current version we're all struggling with.

I've put together a patch that fixes this in my test. .deb available here: https://launchpad.net/~nelhage/+archive/zsnes and I've attached a debdiff.

The attachment "Add a patch that fixes loading save states." seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

Thanks for your work here.

+ * fortify-source-load.patch: New patch to fix loading save files under
+ FORTIFY_SOURCE (LP: 1173090)

The correct syntax for closing a bug report will be LP: #1173090.

-extern unsigned int soundcycleft, spc700read, timer2upd, xa, PHnum2writesfxreg;
-extern unsigned int opcd, HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;
+extern unsigned int soundcycleft, timer2upd, xa, PHnum2writesfxreg;
+extern unsigned char spc700read[], xaread[], opcd[], oamread[];
+extern unsigned int HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;

You turn some variables to arrays, but do not allocate memory anywhere — is that intentional?

+export DEB_BUILD_OPTIONS=nostrip

Why?

Thanks for your prompt review! New patch attached, some comments inline.

> The correct syntax for closing a bug report will be LP: #1173090.

Fixed.

> -extern unsigned int soundcycleft, spc700read, timer2upd, xa, PHnum2writesfxreg;
> -extern unsigned int opcd, HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;
> +extern unsigned int soundcycleft, timer2upd, xa, PHnum2writesfxreg;
> +extern unsigned char spc700read[], xaread[], opcd[], oamread[];
> +extern unsigned int HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;

> You turn some variables to arrays, but do not allocate memory anywhere — is that intentional?

Yes. The basic bug that's triggering the buffer overflow detection is that zsnes declares a block of variables in assembly:

NEWSYM oamaddr, dd 0 ; oam address

NEWSYM bg1ptrx, dd 0 ; pointer to background1
NEWSYM bg2ptrx, dd 0 ; pointer to background2
NEWSYM bg3ptrx, dd 0 ; pointer to background3
NEWSYM bg4ptrx, dd 0 ; pointer to background4

...

And then declares the first in C:

extern unsigned int ..., oamaddr, ...;

And then, when loading a save file, attempts to read the entire block of variables using (effectively):

fread(&oamaddr, 4, 14, fp);

This normally works correctly, since the assembly definitions ensure that the variables are in fact contiguous in memory with "oamaddr". FORTIFY_SOURCE, however, reasonably determines that this code is doing a write of 4*14 bytes into an object of size 4, and aborts. My patch changes this by adding a new symbol -- oamread -- which the C code externs as an array, and then does the load into that. It's not the most elegant solution, but it's minimal and correct.

> +export DEB_BUILD_OPTIONS=nostrip

> Why?

Oops! Left that in from debugging my builds. Removed.

Thanks for fixing this, I prefer to give someone who knows assembler a chance to review this. If we have no such person, I'll sponsor this later.

* Dmitry Shachnev <email address hidden> [130618 11:11]:
> Thanks for fixing this, I prefer to give someone who knows assembler a
> chance to review this. If we have no such person, I'll sponsor this
> later.

Hello,

I had a look as even if it does not affect Debian ATM, I'm interested
in merging these changes. Thanks a lot for investing this issue and
providing a patch!

Here is a quick review.

> --- a/src/cpu/regs.inc
> +++ b/src/cpu/regs.inc
> +NEWSYM oamread

This exports a pointer so that the next 14 ints can be referred to in:

> - copy_func(buffer, &oamaddr, 14*4);
> + copy_func(buffer, oamread, 14*4);

This reads the following data declared in regs.inc:

  - 1 int: oamaddr
  - 8 ints: bg[1-4]ptr[xy]
  - 8 bytes: Voice[0-7]Disable
  - 4 bytes: BG[1-4]16x16t
  - 2 ints: SPC700{read, write}

> --- a/src/init.asm
> +++ b/src/init.asm
> +NEWSYM xaread

Same here, this so that the following copy:

> - copy_func(buffer, &xa, 14*4);
> + copy_func(buffer, xaread, 14*4);

can read the next 14 ints: xa, xdb, xpb, xs, xd, xx, xy, flagnz,
flago, flagc, bankkp, Sflagnz, Sflago and Sflagc.

> --- a/src/gblvars.h
> +++ b/src/gblvars.h
> @@ -27,13 +27,14 @@
> -extern unsigned int soundcycleft, spc700read, timer2upd, xa, PHnum2writesfxreg;
> -extern unsigned int opcd, HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;
> +extern unsigned int soundcycleft, timer2upd, xa, PHnum2writesfxreg;
> +extern unsigned char spc700read[], xaread[], opcd[], oamread[];
> +extern unsigned int HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;

This is equivalent to :

> -extern unsigned int spc700read;
> -extern unsigned int opcd;
> +extern unsigned char spc700read[];
> +extern unsigned char opcd[];
> +extern unsigned char xaread[];
> +extern unsigned char oamread[];

As the symbols spc700read and opcd are now interpreted by address,
their value is equal to the address of the variable, so the following
hunks work:

> - copy_func(buffer, &spc700read, 10*4);
> + copy_func(buffer, spc700read, 10*4);
> copy_func(buffer, &timer2upd, 4);
> copy_func(buffer, &spcnumread, 1);
> - copy_func(buffer, &opcd, 6*4);
> + copy_func(buffer, opcd, 6*4);

However, I think that in the case of char[] variables, you can use &x
for x, so this is unnecessary to remove the & operator (this remark
also applies to oamread and xaread).

> -extern unsigned char sndrot[], SPCRAM[65472], DSPMem[256], SA1Status, *SA1RAMArea;
> +extern unsigned char sndrot[], SPCRAM[65472], DSPMem[256], SA1Status, *SA1RAMArea, *SPCState;
> --- a/src/initc.c
> +++ b/src/initc.c
> +unsigned char *SPCState = SPCRAM;
> --- a/src/zstate.c
> +++ b/src/zstate.c
> - copy_func(buffer, SPCRAM, PHspcsave);
> + copy_func(buffer, SPCState, PHspcsave);

Is a global necessary? If you put this line in copy_spc_data this
should work, unless you have to put it in a different file to "trick"
the static analysis.

Anyway, something seems off with the size of this variable. Cppcheck
detects an error with it:

http://qa.debian.org/daca/cppcheck/sid/zsnes_1.510+bz2-1.html

My guess is that it's related to the SPC ROM located after SPCRAM.
init65816() accesses 0x40 of those bytes through SPCRAM (I'm not sure
that the 16 ones aft...

> However, I think that in the case of char[] variables, you can use &x
> for x, so this is unnecessary to remove the & operator (this remark
> also applies to oamread and xaread).

I think that is true (have not tested). I certainly find the version without & clearer, but it may be worth having a more minimal patch.

> Is a global necessary? If you put this line in copy_spc_data this
> should work, unless you have to put it in a different file to "trick"
> the static analysis.

Without testing, I suspect that GCC will "see through" the separate variable, and the FORTIFY_SOURCE checks will still kick in. Declaring it as a char[] and defining it in the .asm file (like I did for the others) might be cleaner, though.

> Anyway, something seems off with the size of this variable. Cppcheck
> detects an error with it:

Yeah, there are those problems and tons of others that GCC finds -- search https://launchpadlibrarian.net/142716062/buildlog_ubuntu-raring-i386.zsnes_1.510%2Bbz2-5ubuntu2fortify2_UPLOADING.txt.gz for "will always overflow destination buffer". I only fixed the ones that were necessary to make loading a save file work in my testing; Someone should probably dig into the rest, but I didn't have the time to understand the source code well enough to e.g. change the size in the declaration of SPCRAM and be confident that was safe.

OK, even though there are some comments about the patch, I am going to upload it to saucy anyway, and SRU it to raring, as it works and resolves the issue.

We can always revise it with a cleaner version once one's available.

Thanks!

* Nelson Elhage <email address hidden> [130619 09:26]:
> > However, I think that in the case of char[] variables, you can use &x
> > for x, so this is unnecessary to remove the & operator (this remark
> > also applies to oamread and xaread).
>
> I think that is true (have not tested). I certainly find the version
> without & clearer, but it may be worth having a more minimal patch.
>
> > Is a global necessary? If you put this line in copy_spc_data this
> > should work, unless you have to put it in a different file to "trick"
> > the static analysis.
>
> Without testing, I suspect that GCC will "see through" the separate
> variable, and the FORTIFY_SOURCE checks will still kick in. Declaring it
> as a char[] and defining it in the .asm file (like I did for the others)
> might be cleaner, though.
>
> > Anyway, something seems off with the size of this variable. Cppcheck
> > detects an error with it:
>
> Yeah, there are those problems and tons of others that GCC finds --
> search https://launchpadlibrarian.net/142716062/buildlog_ubuntu-
> raring-i386.zsnes_1.510%2Bbz2-5ubuntu2fortify2_UPLOADING.txt.gz for
> "will always overflow destination buffer". I only fixed the ones that
> were necessary to make loading a save file work in my testing; Someone
> should probably dig into the rest, but I didn't have the time to
> understand the source code well enough to e.g. change the size in the
> declaration of SPCRAM and be confident that was safe.

I agree with all you said. I created a bug report on the Debian side
to track the inclusion of this patch: http://bugs.debian.org/712790

When this one and http://bugs.debian.org/698990 will be closed, I
believe that there won't be any divergence between Debian and Ubuntu.

--
Etienne Millon

Uploaded to raring for processing by the SRU team.
Thanks!

This bug was fixed in the package zsnes - 1.510+bz2-5ubuntu3

---------------
zsnes (1.510+bz2-5ubuntu3) saucy; urgency=low

  * fortify-source-load.patch: New patch to fix loading save files under
    FORTIFY_SOURCE (LP: #1173090)
 -- Nelson Elhage <email address hidden> Mon, 17 Jun 2013 22:57:15 -0700

Hello Stéphane, or anyone else affected,

Accepted zsnes into raring-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/zsnes/1.510+bz2-5ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Hello all,

I just downloaded installed and tested the fixed package from:
http://launchpadlibrarian.net/142955218/zsnes_1.510%2Bbz2-5ubuntu2.1_i386.deb

It seems that the bug is gone. zSNES is able to load games again.

Thanks for helping fixing the issue!

Stéphane

This bug was fixed in the package zsnes - 1.510+bz2-5ubuntu2.1

---------------
zsnes (1.510+bz2-5ubuntu2.1) raring; urgency=low

  * fortify-source-load.patch: New patch to fix loading save files under
    FORTIFY_SOURCE (LP: #1173090)
 -- Nelson Elhage <email address hidden> Mon, 17 Jun 2013 22:57:15 -0700

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.