linux-kernel - Re: NULL pointer dereference when access /proc/net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFt=RON1gRRmsZmaeGLEvGoDyJeDksTappF2v8MGQ3rcpi809A@mail.gmail.com>
Date:   Mon, 26 Apr 2021 02:15:37 +0800
From:   haosdent <haosdent@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        zhengyu.duan@...pee.com, Haosong Huang <huangh@....com>
Subject: Re: NULL pointer dereference when access /proc/net

> in RCU mode we really, really should not assume ->d_inode stable.

Got it, but looks like the ->d_inode is NULL after out of RCU.

In `lookup_fast` and `walk_component`

```
  dentry = __d_lookup_rcu(parent, &nd->last, &seq);
  ...
  *inode = d_backing_inode(dentry);
```

```
static int walk_component(struct nameidata *nd, int flags)
  ...
  err = lookup_fast(nd, &path, &inode, &seq);
  if (unlikely(err <= 0)) {
    ...
    path.dentry = lookup_slow(&nd->last, nd->path.dentry, nd->flags);
    ...
    seq = 0; /* we are already out of RCU mode */
    inode = d_backing_inode(path.dentry);
  }
```

On Mon, Apr 26, 2021 at 2:00 AM haosdent <haosdent@...il.com> wrote:
>
> > In the kernels of 4.8..4.18 period there it used to do
> > so, but only in non-RCU mode (which is the reason for explicit rcu argument passed
> > through that callchain).
>
> Yep, we saw the `inode` parameter pass to `__atime_needs_update` is already NULL
>
> ```
> bool __atime_needs_update(const struct path *path, struct inode *inode,
>   bool rcu)
> {
> struct vfsmount *mnt = path->mnt;
> struct timespec now;
>
> if (inode->i_flags & S_NOATIME)   <=== Oops at here because the params
> inode is NULL
> return false;
> ```
>
> ```
>     [exception RIP: __atime_needs_update+5]
>     ...  **RSI: 0000000000000000**  <=== the second params of
> __atime_needs_update "struct inode *inode" is NULL
> ```
>
> On Mon, Apr 26, 2021 at 1:22 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > On Mon, Apr 26, 2021 at 01:04:46AM +0800, haosdent wrote:
> > > Hi, Alexander, thanks a lot for your quick reply.
> > >
> > > > Not really - the crucial part is ->d_count == -128, i.e. it's already past
> > > > __dentry_kill().
> > >
> > > Thanks a lot for your information, we would check this.
> > >
> > > > Which tree is that?
> > > > If you have some patches applied on top of that...
> > >
> > > We use Ubuntu Linux Kernel "4.15.0-42.45~16.04.1" from launchpad directly
> > > without any modification,  the mapping Linux Kernel should be
> > > "4.15.18" according
> > > to https://people.canonical.com/~kernel/info/kernel-version-map.html
> >
> > Umm...  OK, I don't have it Ubuntu source at hand, but the thing to look into
> > would be
> >         * nd->flags contains LOOKUP_RCU
> >         * in the mainline from that period (i.e. back when __atime_needs_update()
> > used to exist) we had atime_needs_update_rcu() called in get_link() under those
> > conditions, with
> > static inline bool atime_needs_update_rcu(const struct path *path,
> >                                           struct inode *inode)
> > {
> >         return __atime_needs_update(path, inode, true);
> > }
> > and __atime_needs_update() passing its last argument (rcu:true in this case) to
> > relatime_need_update() in
> >         if (!relatime_need_update(path, inode, now, rcu))
> > relatime_need_update() hitting
> >         update_ovl_inode_times(path->dentry, inode, rcu);
> > and update_ovl_inode_times() starting with
> >         if (rcu || likely(!(dentry->d_flags & DCACHE_OP_REAL)))
> >                 return;
> > with subsequent accesses to ->d_inode.  Those obviously are *NOT* supposed
> > to be reached in rcu mode, due to that check.
> >
> > Your oops looks like something similar to that call chain had been involved and
> > somehow had managed to get through to those ->d_inode uses.
> >
> > Again, in RCU mode we really, really should not assume ->d_inode stable.  That's
> > why atime_needs_update() gets inode as a separate argument and does *NOT* look
> > at path->dentry at all.  In the kernels of 4.8..4.18 period there it used to do
> > so, but only in non-RCU mode (which is the reason for explicit rcu argument passed
> > through that callchain).
>
>
>
> --
> Best Regards,
> Haosdent Huang



-- 
Best Regards,
Haosdent Huang