question about potential integer truncation in default_erasesize

question about potential integer truncation in default_erasesize

[PaX Team]

hi all,

drivers/mtd/chips/map_rom.c:default_erasesize can truncate map_info.size
from unsigned long to unsigned int on 64 bit archs and i'm wondering if
this is intentional or should/could map_info.size be turned into an unsigned
int field? FTR, this issue was detected with the upcoming version of the
size overflow plugin we have in PaX/grsecurity and there're a handful of
similar cases in the tree where potentially unwanted or unnecessary integer
truncations occur, this being one of these. any opinion/help is welcome!

cheers,
  PaX Team

Re: question about potential integer truncation in default_erasesize

[Brian Norris]

On Sat, Sep 26, 2015 at 03:18:08PM +0200, PaX Team wrote:
> hi all,
> 
> drivers/mtd/chips/map_rom.c:default_erasesize can truncate map_info.size
> from unsigned long to unsigned int on 64 bit archs and i'm wondering if
> this is intentional or should/could map_info.size be turned into an unsigned
> int field? FTR, this issue was detected with the upcoming version of the
> size overflow plugin we have in PaX/grsecurity and there're a handful of
> similar cases in the tree where potentially unwanted or unnecessary integer
> truncations occur, this being one of these. any opinion/help is welcome!

This is being assigned to the erasesize, which is 32-bit already, and
all of MTD expects a 32-bit erasesize, so it'd be a pretty big job to
"fix" the truncation. But really, we can't handle (and shouldn't;
there's really no need) >32-bit eraseblocks. That would be an
un-manageable flash geometry.

Regards,
Brian

Re: question about potential integer truncation in default_erasesize

[PaX Team]

On 28 Sep 2015 at 19:02, Brian Norris wrote:

> On Sat, Sep 26, 2015 at 03:18:08PM +0200, PaX Team wrote:
> > hi all,
> > 
> > drivers/mtd/chips/map_rom.c:default_erasesize can truncate map_info.size
> > from unsigned long to unsigned int on 64 bit archs and i'm wondering if
> > this is intentional or should/could map_info.size be turned into an unsigned
> > int field? FTR, this issue was detected with the upcoming version of the
> > size overflow plugin we have in PaX/grsecurity and there're a handful of
> > similar cases in the tree where potentially unwanted or unnecessary integer
> > truncations occur, this being one of these. any opinion/help is welcome!
> 
> This is being assigned to the erasesize, which is 32-bit already, and
> all of MTD expects a 32-bit erasesize, so it'd be a pretty big job to
> "fix" the truncation.

to make sure i got this right, map_info.size in other uses can hold a
value larger than 4GB (so it has to stay 64 bit, at least on 64 bit
archs) but for erasesize it should never have such a big value? that'd
actually be fine for us since it means when the overflow plugin instruments
this code any runtime trigger means a real problem, not a false positive.

cheers,
 PaX Team

Re: question about potential integer truncation in default_erasesize

[Brian Norris]

On Tue, Sep 29, 2015 at 10:56:25PM +0200, PaX Team wrote:
> On 28 Sep 2015 at 19:02, Brian Norris wrote:
> 
> > On Sat, Sep 26, 2015 at 03:18:08PM +0200, PaX Team wrote:
> > > hi all,
> > > 
> > > drivers/mtd/chips/map_rom.c:default_erasesize can truncate map_info.size
> > > from unsigned long to unsigned int on 64 bit archs and i'm wondering if
> > > this is intentional or should/could map_info.size be turned into an unsigned
> > > int field? FTR, this issue was detected with the upcoming version of the
> > > size overflow plugin we have in PaX/grsecurity and there're a handful of
> > > similar cases in the tree where potentially unwanted or unnecessary integer
> > > truncations occur, this being one of these. any opinion/help is welcome!
> > 
> > This is being assigned to the erasesize, which is 32-bit already, and
> > all of MTD expects a 32-bit erasesize, so it'd be a pretty big job to
> > "fix" the truncation.
> 
> to make sure i got this right, map_info.size in other uses can hold a
> value larger than 4GB (so it has to stay 64 bit, at least on 64 bit
> archs) but for erasesize it should never have such a big value?

Correct.

> that'd
> actually be fine for us since it means when the overflow plugin instruments
> this code any runtime trigger means a real problem, not a false positive.

OK, good. As long as you aren't going to start complaining about
theoretical concerns we're OK, but a dynamic check is cool.

Brian

Re: question about potential integer truncation in default_erasesize

[PaX Team]

On 29 Sep 2015 at 14:02, Brian Norris wrote:

> > that'd
> > actually be fine for us since it means when the overflow plugin instruments
> > this code any runtime trigger means a real problem, not a false positive.
> 
> OK, good. As long as you aren't going to start complaining about
> theoretical concerns we're OK, but a dynamic check is cool.

we don't care about theoretical bugs but real ones too. the reason for asking 
about this code is that while investigating a false positive report in some
other code (that turned out to be due to a limitation in how gcc represents
certain constructs at its gimple layer) we decided to check the underlying
cause in the whole tree so that we could decide how to fix the FP. the choice
is usually either changing the source code (so that gcc produces a different
representation) or disabling instrumentation for either the particular
expression or all similar constructs. the decision comes down to how many
potentially useful checks we'd omit vs. how many more false positives we'd
risk having. in this case the code construct was copying between structure
fields of different sizes and linux has fortunately a few of these only that
can be manually verified, save for a few cases i could not decide myself, hence
this email and few others elsewhere (some turned out to be real bugs or just
inefficient code in fact).

in short, if you ever hear from us again it'll be because there was a real
integer truncation/overflow/etc event (and we'll have precise source location
for it ;).