Null pointer dereference in ngene-core.c

Null pointer dereference in ngene-core.c

[Alexandre-Xavier Labonté-Lamoureux]

Hi people,

In the file "/linux/drivers/media/pci/ngene/ngene-core.c", there is a
null pointer dereference at line 1480.

Code in the function "static int init_channel(struct ngene_channel *chan)"
======================================
if (io & NGENE_IO_TSIN) {
    chan->fe = NULL;                      // Set to NULL
    if (ni->demod_attach[nr]) {         // First condition
       ret = ni->demod_attach[nr](chan);
            if (ret < 0)                           // Another condition
                goto err;                         // Goto that avoids
the problem
    }
    if (chan->fe && ni->tuner_attach[nr]) {     // Condition that
tests the null pointer
        ret = ni->tuner_attach[nr](chan);
        if (ret < 0)
            goto err;
    }
}
=====================================

"chan->fe" is set to NULL, then it tests for something (I have no idea
what it's doing, I know nothing about this driver), if the results of
the first two if conditions fail to reach the goto, then it will test
the condition with the null pointer, which will cause a crash. I don't
know if the kernel can recover from null pointers, I think not.

--Alexandre-Xavier

Re: Null pointer dereference in ngene-core.c

[Devin Heitmueller]

On Mon, Sep 19, 2016 at 8:51 PM, Alexandre-Xavier Labonté-Lamoureux
<axdoomer@gmail.com> wrote:
> Hi people,
>
> In the file "/linux/drivers/media/pci/ngene/ngene-core.c", there is a
> null pointer dereference at line 1480.
>
> Code in the function "static int init_channel(struct ngene_channel *chan)"
> ======================================
> if (io & NGENE_IO_TSIN) {
>     chan->fe = NULL;                      // Set to NULL
>     if (ni->demod_attach[nr]) {         // First condition
>        ret = ni->demod_attach[nr](chan);
>             if (ret < 0)                           // Another condition
>                 goto err;                         // Goto that avoids
> the problem
>     }
>     if (chan->fe && ni->tuner_attach[nr]) {     // Condition that
> tests the null pointer
>         ret = ni->tuner_attach[nr](chan);
>         if (ret < 0)
>             goto err;
>     }
> }
> =====================================
>
> "chan->fe" is set to NULL, then it tests for something (I have no idea
> what it's doing, I know nothing about this driver), if the results of
> the first two if conditions fail to reach the goto, then it will test
> the condition with the null pointer, which will cause a crash. I don't
> know if the kernel can recover from null pointers, I think not.

This looks fine to me.  It's a simple test to see if chan->fe got set
to null (presumably in the above block of code).  A null pointer
dereference would be if the first block set *chan* to NULL (as opposed
to chan->fe) and then the if() statement then attempted to inspect
chan->fe.

LGTM.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: Null pointer dereference in ngene-core.c

[Devin Heitmueller]

On Mon, Sep 19, 2016 at 8:51 PM, Alexandre-Xavier Labonté-Lamoureux
<axdoomer@gmail.com> wrote:
> Hi people,
>
> In the file "/linux/drivers/media/pci/ngene/ngene-core.c", there is a
> null pointer dereference at line 1480.
>
> Code in the function "static int init_channel(struct ngene_channel *chan)"
> ======================================
> if (io & NGENE_IO_TSIN) {
>     chan->fe = NULL;                      // Set to NULL
>     if (ni->demod_attach[nr]) {         // First condition
>        ret = ni->demod_attach[nr](chan);
>             if (ret < 0)                           // Another condition
>                 goto err;                         // Goto that avoids
> the problem
>     }
>     if (chan->fe && ni->tuner_attach[nr]) {     // Condition that
> tests the null pointer
>         ret = ni->tuner_attach[nr](chan);
>         if (ret < 0)
>             goto err;
>     }
> }
> =====================================
>
> "chan->fe" is set to NULL, then it tests for something (I have no idea
> what it's doing, I know nothing about this driver), if the results of
> the first two if conditions fail to reach the goto, then it will test
> the condition with the null pointer, which will cause a crash. I don't
> know if the kernel can recover from null pointers, I think not.

I would have to actually look at the code, but my guess is because the
call to ni-ni->demod_attach[nr](chan) will actually set chan->fe if
successful.

The code path your describing is actually the primary use case.  The
cases where you see "goto err" will only be followed if there was some
sort of error condition, which means the driver essentially will
*always* hit the if() statement you are referring to during normal
operation (assuming nothing was broken in the hardware).

In short, the code makes sure chan->fe is NULL, then it calls the
demod_attach, then it checks both for the demod_attach returning an
error *and* making sure demod_attach set chan->fe to some non-NULL
value.  If both are the case, then it calls tuner_attach().

This is a pretty standard workflow -- you should see it in many other
drivers, although it's not uncommon in other drivers for something
like chan->fe to actually be the value returned by the demod_attach(),
and the demod attach routine would return NULL if there was some
failure condition.  The problem with that approach is that you can
only report one type of failure to the caller (all the caller knows is
a failure occured, it has no visibility as to the nature of the
failure), whereas with this approach you can return different values
for different error conditions.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com