[CHECKER] 3 kmalloc underallocation bugs

[CHECKER] 3 kmalloc underallocation bugs

[Dawson Engler]

enclosed are three bugs found in the 2.4.1 kernel by an extension
that checks that kmalloc calls allocate enough memory.  It examines all
callsites of the form:
	p = [kv]malloc(nbytes);
and issues an error if
	sizeof *p < nbytes

I think they're all currently harmless because of kmalloc & friends
exuberant approach to padding.

Dawson

drivers/sound/emu10k1/midi.c
drivers/telephony/ixj.c
---------------------------------------------------------
[BUG]  should allocate sizeof *midihdr

/u2/engler/mc/oses/linux/2.4.1/drivers/sound/emu10k1/midi.c:59:midiin_add_buffer
: ERROR:SIZE-CHECK:59:59: midihdr = 'kmalloc'(4 bytes), need 32


static int midiin_add_buffer(struct emu10k1_mididevice *midi_dev, struct midi_hd
r **midihdrptr)
{
        struct midi_hdr *midihdr;

Error --->
        if ((midihdr = (struct midi_hdr *) kmalloc(sizeof(struct midi_hdr *), GF
P_KERNEL)) == NULL) {
                ERROR();
                return -EINVAL;
        }

---------------------------------------------------------
[BUG] same
/u2/engler/mc/oses/linux/2.4.1/drivers/sound/emu10k1/midi.c:331:emu10k1_midi_wri
te: ERROR:SIZE-CHECK:331:331: midihdr = 'kmalloc'(4 bytes), need 32

        struct midi_hdr *midihdr;
        ssize_t ret = 0;
        unsigned long flags;

        DPD(4, "emu10k1_midi_write(), count=%x\n", (u32) count);

        if (pos != &file->f_pos)
                return -ESPIPE;

        if (!access_ok(VERIFY_READ, buffer, count))
                return -EFAULT;

Error --->
        if ((midihdr = (struct midi_hdr *) kmalloc(sizeof(struct midi_hdr *), GF
P_KERNEL)) == NULL)
                return -EINVAL;

---------------------------------------------------------
[BUG]  should be sizeof(IXJ_FILTER_CADENCE) as with the copy_from_user

/u2/engler/mc/oses/linux/2.4.1/drivers/telephony/ixj.c:4511:ixj_build_filter_cad
ence: ERROR:SIZE-CHECK:4511:4511: lcp = 'kmalloc'(12 bytes), need 32


        ... DELETED 7 lines ...

        IXJ_FILTER_CADENCE *lcp;
        IXJ *j = &ixj[board];
Error --->
        lcp = kmalloc(sizeof(IXJ_CADENCE), GFP_KERNEL);
        if (lcp == NULL)
                return -ENOMEM;
        if (copy_from_user(lcp, (char *) cp, sizeof(IXJ_FILTER_CADENCE)))
                return -EFAULT;
----------------------------------------

Re: [CHECKER] 3 kmalloc underallocation bugs

[André Dahlqvist]

Dawson Engler <engler@csl.Stanford.EDU> wrote:
> enclosed are three bugs found in the 2.4.1 kernel by an extension

Why are you guys running these tests against an already old kernel?
I would suggest running it against at least Linus' latest version, or
preferably Alan's -ac tree.
-- 

André Dahlqvist <anedah-9@sm.luth.se>

Re: [CHECKER] 3 kmalloc underallocation bugs

[Jeff Golds]

André Dahlqvist wrote:
>
> Dawson Engler <engler@csl.Stanford.EDU> wrote:
> > enclosed are three bugs found in the 2.4.1 kernel by an extension
>
> Why are you guys running these tests against an already old kernel?
> I would suggest running it against at least Linus' latest version, or
> preferably Alan's -ac tree.

At least the two bugs in emu10k1/midi.c still exist in 2.4.3.

Just because 2.4.3 is a later version, doesn't mean all the bugs are
fixed from earlier versions.

-Jeff

-- 
Jeff Golds
jgolds@resilience.com

Re: [CHECKER] 3 kmalloc underallocation bugs

[Ben Pfaff]

Dawson Engler <engler@csl.Stanford.EDU> writes:

> enclosed are three bugs found in the 2.4.1 kernel by an extension
> that checks that kmalloc calls allocate enough memory.  It examines all
> callsites of the form:
> 	p = [kv]malloc(nbytes);
> and issues an error if
> 	sizeof *p < nbytes

[...]

>         struct midi_hdr *midihdr;
> 
> Error --->
>         if ((midihdr = (struct midi_hdr *) kmalloc(sizeof(struct midi_hdr *), GF
> P_KERNEL)) == NULL) {

This sort of thing is why the comp.lang.c approved way to call
malloc() is
	foo *x = malloc (sizeof *x);
No cast is required and the sizeof usage resembles the
declaration.  The following is what I say on comp.lang.c when
someone does it another way.  AFAICS the recommendations apply
equally to [kv]malloc().
----------------------------------------------------------------------
When calling malloc(), I recommend using the sizeof operator on the
object you are allocating, not on the type.  For instance, *don't*
write this:

	int *x = malloc (sizeof (int) * 128); /* Don't do this! */

Instead, write it this way:

	int *x = malloc (sizeof *x * 128);

There's a few reasons to do it this way:

	* If you ever change the type that `x' points to, it's not
          necessary to change the malloc() call as well.  

	  This is more of a problem in a large program, but it's still
	  convenient in a small one.

	* Taking the size of an object makes your sizeof call more
          similar to your declaration, which makes writing the
          statement less error-prone.  

	  For instance, above, the declaration syntax is `*x' and the
	  sizeof operation is also written `*x'.  This provides a
	  visual clue that the malloc() call is correct.

I don't recommend casting the return value of malloc():

	* The cast is not required in ANSI C.

	* Casting its return value can mask a failure to #include
          <stdlib.h>, which leads to undefined behavior.

	* If you cast to the wrong type by accident, odd failures can
	  result.
----------------------------------------------------------------------

-- 
Ben Pfaff <pfaffben@msu.edu> <pfaffben@debian.org> <blp@gnu.org>
MSU Student - Debian GNU/Linux Maintainer - GNU Developer
Personal webpage: http://www.msu.edu/user/pfaffben