Subject: Re: UFS quota null pointer dereference
To: None <tech-kern@netbsd.org>
From: Christos Zoulas <christos@astron.com>
List: tech-kern
Date: 06/25/2007 22:34:29
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