[MTD] potential division by 0 in cfi_build_cmd() and cfi_merge_status()?

[MTD] potential division by 0 in cfi_build_cmd() and cfi_merge_status()?

[Roel Kluin]

Doing some grepping, I stumbled upon this possible error:

in include/linux/mtd/cfi.h, lines 302 and 366, resp. functions
cfi_build_cmd() and cfi_merge_status() there is a division by
cfi_interleave(cfi):

chip_mode = map_bankwidth(map) / cfi_interleave(cfi);

This could be problematic when No CONFIG_MTD_CFI_Ix is selected:
cfi_interleave will triggers BUG(), but when BUG is disabled, the
function returns 0, causing a subsequent division by zero.

When a CONFIG_MTD_CFI_Ix is selected, cfi_interleave(cfi) is either
defined 1 or defined (cfi)->interleave.

cfi is a struct cfi_private pointer, with interleave as an int.

I am not sure whether interleave can ever be 0 in this division when 
CONFIG_MTD_CFI_Ix is set.

shouldn't there be an error exit when cfi_interleave(cfi) evaluates
to 0?

I am not subscribed to this list, so please CC.

Roel

Re: [MTD] potential division by 0 in cfi_build_cmd() and cfi_merge_status()?

[Jörn Engel]

On Tue, 15 January 2008 01:28:12 +0100, Roel Kluin wrote:
> 
> Doing some grepping, I stumbled upon this possible error:
> 
> in include/linux/mtd/cfi.h, lines 302 and 366, resp. functions
> cfi_build_cmd() and cfi_merge_status() there is a division by
> cfi_interleave(cfi):
> 
> chip_mode = map_bankwidth(map) / cfi_interleave(cfi);
> 
> This could be problematic when No CONFIG_MTD_CFI_Ix is selected:
> cfi_interleave will triggers BUG(), but when BUG is disabled, the
> function returns 0, causing a subsequent division by zero.
> 
> When a CONFIG_MTD_CFI_Ix is selected, cfi_interleave(cfi) is either
> defined 1 or defined (cfi)->interleave.
> 
> cfi is a struct cfi_private pointer, with interleave as an int.
> 
> I am not sure whether interleave can ever be 0 in this division when 
> CONFIG_MTD_CFI_Ix is set.
> 
> shouldn't there be an error exit when cfi_interleave(cfi) evaluates
> to 0?

I don't think cfi_interleave(cfi) will ever be 0.  But the functions
definitely look a bit large for inlines.  Anyone having both cfi_probe
and jedec_probe will enjoy twice the kernel footprint from them.
Patches to move that code out-of-line are welcome.

Jörn

-- 
Fools ignore complexity.  Pragmatists suffer it.
Some can avoid it.  Geniuses remove it.
-- Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Re: [MTD] potential division by 0 in cfi_build_cmd() and cfi_merge_status()?

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 731 bytes --]

On Tue, 15 Jan 2008, Jörn Engel wrote:

> I don't think cfi_interleave(cfi) will ever be 0.  But the functions
> definitely look a bit large for inlines.  Anyone having both cfi_probe
> and jedec_probe will enjoy twice the kernel footprint from them.
> Patches to move that code out-of-line are welcome.

Beware.

The typical use case is for your embedded system to be configured with 
your flash arrangement and not all possible combinations available under 
the sun.

In that case it is most likely that cfi_interleave(cfi) will simplify to 
a constant, and making this out of line _will_ enlarge your kernel, as 
well as possibly breaking interaction with a XIP kernel on ARM.

So please don't push this out of line.


Nicolas

Re: [MTD] potential division by 0 in cfi_build_cmd() and cfi_merge_status()?

[Jörn Engel]

On Mon, 14 January 2008 23:06:13 -0500, Nicolas Pitre wrote:
> On Tue, 15 Jan 2008, Jörn Engel wrote:
> 
> > I don't think cfi_interleave(cfi) will ever be 0.  But the functions
> > definitely look a bit large for inlines.  Anyone having both cfi_probe
> > and jedec_probe will enjoy twice the kernel footprint from them.
> > Patches to move that code out-of-line are welcome.
> 
> Beware.
> 
> The typical use case is for your embedded system to be configured with 
> your flash arrangement and not all possible combinations available under 
> the sun.
> 
> In that case it is most likely that cfi_interleave(cfi) will simplify to 
> a constant, and making this out of line _will_ enlarge your kernel, as 
> well as possibly breaking interaction with a XIP kernel on ARM.
> 
> So please don't push this out of line.

Fair enough.

Jörn

-- 
Joern's library part 13:
http://www.chip-architect.com/