x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11894:be1b22d0c36a

---------------------------

x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils
the compiler seems to align the fp80_t data struct, so here we add

explicit padding to avoid confusion.
storeFloat80() will try to write all 16B of the fp80_t to the bits[] array

of the calling instruction. this happens because storeFloat80() points its

local fp80_t* to the memory the caller allocated for bits[], which is only

10B, thus we get an overflow that is flagged by clang's asan. here we

get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem

allocated by the caller.
some of the x86 FP ops also use char to represent 8b types, while the fp80

struct uses uint8_t, so here we make the x86 ops use uint8_t as well.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I would prefer it if we could avoid bit annotation here. The C spec seems to specify that: "The order ...	Andreas Sandberg	Nov. 15, 2016, 9:17 a.m.	Resolved
Are these changes really needed?	Andreas Sandberg	Nov. 15, 2016, 11:52 a.m.	Resolved

Description:

~		Changeset 11858:9400f22ebf66
	~	Changeset 11858:6f811e3a77a9

		x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

		the compiler seems to align the fp80_t data struct, so here we add
		explicit padding to avoid confusion.

		storeFloat80() will try to write all 16B of the fp80_t to the bits[] array
		of the calling instruction. this happens because storeFloat80() points its
		local fp80_t* to the memory the caller allocated for bits[], which is only
		10B, thus we get an overflow that is flagged by clang's asan. here we
		get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem
		allocated by the caller.

Diff:

Revision 2 (+3 -3)

Show changes

	ext/fputils/include/fputils/fptypes.h
	src/arch/x86/utility.cc

Description:

~		Changeset 11858:6f811e3a77a9
~
~		x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils
	~	Changeset 11858:6f811e3a77a9
	~	---------------------------
	~	x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

		the compiler seems to align the fp80_t data struct, so here we add
		explicit padding to avoid confusion.

		storeFloat80() will try to write all 16B of the fp80_t to the bits[] array
		of the calling instruction. this happens because storeFloat80() points its
		local fp80_t* to the memory the caller allocated for bits[], which is only
		10B, thus we get an overflow that is flagged by clang's asan. here we
~		get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem
	~	get the fp80 value first, then memcpy() the bits[] of fp80_t to the mem
		allocated by the caller.

Description:

~		Changeset 11858:6f811e3a77a9
	~	Changeset 11888:ed39f10e4ff3
		---------------------------
		x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

		the compiler seems to align the fp80_t data struct, so here we add
		explicit padding to avoid confusion.

		storeFloat80() will try to write all 16B of the fp80_t to the bits[] array
		of the calling instruction. this happens because storeFloat80() points its
		local fp80_t* to the memory the caller allocated for bits[], which is only
		10B, thus we get an overflow that is flagged by clang's asan. here we
~		get the fp80 value first, then memcpy() the bits[] of fp80_t to the mem
	~	get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem
		allocated by the caller.

Diff:

Revision 3 (+11 -9)

Show changes

	ext/fputils/fpbits.h
	ext/fputils/include/fputils/fptypes.h
	src/arch/x86/utility.cc
	src/arch/x86/isa/microops/fpop.isa

Tested on X86 booting FreeBSD reverting my own simple workaround.
I can say that gem5 does no longer panic when booting FreeBSD FS with this patch applied.
I cannot say whether it works correctly currently.
However it improves the situation :)

Tony Gutierrez Nov. 10, 2016, 1:08 p.m. (Nov. 10, 2016, 1:08 p.m.)

Great. I'd like to ship this soon, but I'd like to see what Andreas S. thinks of it first.

Andreas, do you have any comments/concerns with this fix?

Any thoughts? Otherwise I'll ship this soon.

Thanks for fixing this. The only technical issue I have with this patch is that it uses bit fields, which have slightly undefined semantics. I would like to avoid that if possible. I think it would be fine to just add a uitn8_t array instead after se.

A non-technical issue is that we should make sure this is included in the upstream library and then pulled into ext. Could you post a pull request to https://github.com/andysan/libfputils ?

Tony Gutierrez Nov. 15, 2016, 9:18 a.m. (Nov. 15, 2016, 9:18 a.m.)
```
Sure. Are you asking me to not push this into gem5 though?
```

Andreas Sandberg Nov. 15, 2016, 9:24 a.m. (Nov. 15, 2016, 9:24 a.m.)

I would prefer if we could push it to the external library first and then pull in that revision into ext. This makes it easier to track the upstream revision in the change log.

ext/fputils/include/fputils/fptypes.h (Diff revision 3)

I would prefer it if we could avoid bit annotation here. The C spec seems to specify that: "The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined."

A better solution would probably be to add a 6 byt uint8_t pad array.

Show all issues

Description:

~		Changeset 11888:ed39f10e4ff3
	~	Changeset 11894:c211fc37d7a1
		---------------------------
		x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

		the compiler seems to align the fp80_t data struct, so here we add
		explicit padding to avoid confusion.

		storeFloat80() will try to write all 16B of the fp80_t to the bits[] array
		of the calling instruction. this happens because storeFloat80() points its
		local fp80_t* to the memory the caller allocated for bits[], which is only
		10B, thus we get an overflow that is flagged by clang's asan. here we
		get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem
		allocated by the caller.
	+
	+	some of the x86 FP ops also use char to represent 8b types, while the fp80
	+	struct uses uint8_t, so here we make the x86 ops use uint8_t as well.

Diff:

Revision 4 (+13 -8)

Show changes

	ext/fputils/fpbits.h
	ext/fputils/include/fputils/fptypes.h
	src/arch/x86/utility.cc
	src/arch/x86/isa/microops/fpop.isa

src/arch/x86/isa/microops/fpop.isa (Diff revision 4)

Are these changes really needed?

Show all issues

Tony Gutierrez Nov. 15, 2016, 11:55 a.m. (Nov. 15, 2016, 11:55 a.m.)

Fixed. I will push this once the changes to fputils have been pulled into gem5.

Description:

~		Changeset 11894:c211fc37d7a1
	~	Changeset 11894:be1b22d0c36a
		---------------------------
		x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

		the compiler seems to align the fp80_t data struct, so here we add
		explicit padding to avoid confusion.

		storeFloat80() will try to write all 16B of the fp80_t to the bits[] array
		of the calling instruction. this happens because storeFloat80() points its
		local fp80_t* to the memory the caller allocated for bits[], which is only
		10B, thus we get an overflow that is flagged by clang's asan. here we
		get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem
		allocated by the caller.

		some of the x86 FP ops also use char to represent 8b types, while the fp80
		struct uses uint8_t, so here we make the x86 ops use uint8_t as well.

Diff:

Revision 5 (+5 -5)

Show changes

src/arch/x86/utility.cc

Ship It!

Tony Gutierrez Nov. 18, 2016, 9:30 a.m. (Nov. 18, 2016, 9:30 a.m.)

Would you mind pulling in the changes from fputils? I have no experience doing things like that. Or, if you could send me a mail offline with details about how to go about doing so, I can give it a try. Thanks.

Andreas Sandberg Nov. 18, 2016, 10:16 a.m. (Nov. 18, 2016, 10:16 a.m.)

I generally just checkout the latest revision and copy the files to the HG repo and sort out the commit message manually. I really ought to automate that at some point... I have pushed the latest version now, so this change should be good to go.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11858:9400f22ebf66

Changeset 11858:6f811e3a77a9

Diff:

Description:

Changeset 11858:6f811e3a77a9

Description:

Diff:

Description:

Diff:

Description:

Diff:

Status: Closed (submitted)