-1

I was reading this article http://blog.regehr.org/archives/213 It contains an example at the bottom of the page from the Linux kernel (slightly edited)

static void __devexit agnx_pci_remove (struct pci_dev *pdev)
{
  struct ieee80211_hw *dev = pci_get_drvdata(pdev);
  struct agnx_priv *priv = dev->priv; 

  if (!dev) return;

  ... do stuff using dev ...
}

The article claims

As we can now easily see, neither case necessitates a null pointer check. The check is removed, potentially creating an exploitable security vulnerability.

If they dereferenced a null pointer would it not segfault? They would not even get to the check right?

What can this vulnerability be?

EDIT: I read the article and I understand it! I want to understand why people coded it this way anyway, perhaps it was deliberate? At least for me just because a guy claims it to be a mistake on his blog, does not mean that it is, so I want to double check it. What's wrong with that?

user10607
  • 3,011
  • 4
  • 24
  • 31
  • 5
    This question appears to be off-topic because it is about a simple mis-reading of the relevant article. – simonzack Nov 17 '14 at 09:39
  • Redeferencing virtual memory address 0 does not necessarily yield a segfault.It depends on the configured access permissions of the executable image, which may well be RO (or even RW) for that address. – barak manos Nov 17 '14 at 09:39
  • 1
    Maybe if you read careful all the paragraph you will get all the answers. – Michele d'Amico Nov 17 '14 at 09:50
  • "why people coded it this way" Maybe one or two coders where trying to "initialize all variables" and another "test all inputs", but in the wrong order. – chux - Reinstate Monica Nov 17 '14 at 13:09

2 Answers2

2

If they dereferenced a null pointer would it not segfault?

Yes, and there is a note:

The idiom here is to get a pointer to a device struct, test it for null, and then use it. But there’s a problem! In this function, the pointer is dereferenced before the null check.

David Ranieri
  • 39,972
  • 7
  • 52
  • 94
  • I read that paragraph twice. Linux kernel has many interesting tricks, just because some one thinks that this is a mistake, does not 100% prove it to be so. Are you *sure* that it was just an oversight of the developers and not there for some reason? – user10607 Nov 17 '14 at 09:51
  • @user10607, I think it must be an oversight, take a look to [What happens in OS when we dereference a NULL pointer in C?](http://stackoverflow.com/q/12645647/1606345) – David Ranieri Nov 17 '14 at 10:03
  • @user10607 Undefined behavior is undefined. A kernel programmer that thinks they can predict what a compiler will do, or what future versions of the compiler will do, to a program that invokes undefined behavior is wrong. So if it was written so intentionally, it is still a mistake, just a deeper one that requires education of the developer who intentionally put it in. Also John Regehr is not just “some one” and he is not the only person to think that the Linux source is in the wrong. Every GCC developer who could have changed GCC not to require a special flag to emit the intended code agrees. – Pascal Cuoq Nov 17 '14 at 10:19
0

What's wrong with that?

By dereferencing the pointer before the check for NULL the compiler is allowed to think: "oh, the pointer got dereferenced, so at this point of the program it can not be NULL, otherwise that dereference would have worked. So I can safely omit that whole code path."

On the thinking of the person who wrote that? No idea, this is bad style anyway. The first thing every function with an exposed symbol should do is validate its input (short helper functions being static inline may be exempt from that rule, but the caller of these functions must make sure, all parameters are valid then).

In practice this means, that first thing first all the pointers in the structures and values passed to the function should be checked for NULL in some kind of tree-traversal method. Some people prefer breadth first, I normally use depth first. The important part is, that every pointer is checked for NULL before dereferencing.

datenwolf
  • 159,371
  • 13
  • 185
  • 298