Subject: Re: UFS quota null pointer dereference
To: None <tech-kern@netbsd.org>
From: Juergen Hannken-Illjes <hannken@eis.cs.tu-bs.de>
List: tech-kern
Date: 06/26/2007 10:37:00
On Mon, Jun 25, 2007 at 10:34:29PM +0000, Christos Zoulas wrote:
> In article <20070625153512.GA21802@bseis.eis.cs.tu-bs.de>,
> Juergen Hannken-Illjes  <hannken@eis.cs.tu-bs.de> wrote:
> >On Mon, Jun 25, 2007 at 02:30:02PM +0000, Christos Zoulas wrote:
> >> In article <20070625140234.GA18006@bseis.eis.cs.tu-bs.de>,
> >> Juergen Hannken-Illjes  <hannken@eis.cs.tu-bs.de> wrote:
> >> >In -current we had `struct dquot' in file sys/ufs/ufs/quota.h as:
> >> >
> >> >struct dquot {
> >> >        LIST_ENTRY(dquot) dq_hash;      /* hash list */
> >> >        TAILQ_ENTRY(dquot) dq_freelist; /* free list */
> >> >        u_int16_t dq_flags;             /* flags, see below */
> >> >        u_int16_t dq_cnt;               /* count of active references */
> >> >        u_int16_t dq_spare;             /* unused spare padding */
> >> >        u_int16_t dq_type;              /* quota type of this dquot */
> >> >        u_int32_t dq_id;                /* identifier this applies to */
> >> >        struct  ufsmount *dq_ump;       /* filesystem that this is
> >taken from */
> >> >        struct  dqblk dq_dqb;           /* actual usage & quotas */
> >> >};
> >> >
> >> >This leads to a null pointer derefence if a quota-enabled file system
> >> >has 65536 active vnodes for one uid becaus `dq_cnt' overflows.
> >> >
> >> >So in -current I changed the type of `dq_cnt' to `u_int32_t'.
> >> >Unfortunately this cannot be pulled up to netbsd-2-x and netbsd-3-x
> >> >because it breaks backwards compatibility with 3rd party LKM's.
> >> >
> >> >A quick hack would be to split the counter into two fields:
> >> >
> >> >	u_int16_t dq_cnt;               /* count of active references low */
> >> >-       u_int16_t dq_spare;             /* unused spare padding */
> >> >+       u_int16_t dq_cnt_upper;         /* count of active references high */
> >> >
> >> >and change sys/ufs/ufs/ufs_quota.c to care for this split counter.
> >> >
> >> >Opinions?
> >> 
> >> Is the LKM compatibility issue real? I.e. are you aware of any lkm that
> >> depends on dq_cnt being 16 bit, or even an lkm that uses struct dquot?
> >> If there isn't, I think that it is pretty safe to make it a 32 bit number
> >> and pull it up as such.
> >> 
> >> christos
> >
> >I'm not aware of any such lkm.  `struct dquot' and its consumers in
> >ufs_quota.c should be private but unfortunately they are exported via
> >`quota.h'.
> 
> Just make the change then and put a note in the release notes for good
> measure.
> 
> christos

Agreed.
We use struct dquot members only from sys/ufs/ufs/ufs_quota.c.
If a 3rd party LKM would use these members, the quick hack proposed above
would fail too.
An LKM will fail to load if the kernel version doesn't match so we can expect
admins to think twice before they use `modload -f'.

Releng: ok?
-- 
Juergen Hannken-Illjes - hannken@eis.cs.tu-bs.de - TU Braunschweig (Germany)