kernel/997: Crash in chkdq (/usr/src/sys/ufs/ufs/ufs_quota.c) on alpha

[goeran@cdg.chalmers.se]

>Number:         997
>Category:       kernel
>Synopsis:       Crash in chkdq (/usr/src/sys/ufs/ufs/ufs_quota.c) on alpha
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    bugs
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Dec  2 08:50:01 MST 1999
>Last-Modified:
>Originator:     Goran Bengtson
>Organization:
Chalmers Univ. of Technology, Sweden
>Release:        OpenBSD 2.6
>Environment:
	System      : OpenBSD 2.6
	Architecture: OpenBSD.alpha
	Machine     : alpha
>Description:
	A kernel (2.6-current, or 2.6) built with QUOTA and FFS_SOFTUPDATES,
	running on an alpha with mounted filesystems using softupdates will
	crash at line 137 in file /usr/src/sys/ufs/ufs/ufs_quota.c (routine
	chkdq).

	Line is (ufs_quota.c rev. 1.8):
        	if ((flags & FORCE) == 0 && cred->cr_uid != 0) {

	The reason is the change in /usr/src/sys/ufs/ufs/dinode.h on
	Nov 17'th when di_blocks in struct dinode was changed from int32_t
	to u_int32_t.

	The reason behind the change may be good, but di_blocks is used
	(directly or as result of defines in /usr/src/sys/ufs/ufs/inode.h,
	e.g.  "#define  i_ffs_blocks            i_din.ffs_din.di_blocks"
	together with signed variable in several kernel filesystem-related
	routines.  This may lead to unexpexted behaviour unless the code is
	checked, at least if the value of di_blocks is large enough to
	motivate it beeing unsigned.

	One such problem cause the crash above, where chkdq was called from
	line 237 in file /usr/src/sys/ufs/ffs/ffs_inode.c (rev. 1.10).
	Line is:
		(void) chkdq(oip, -oip->i_ffs_blocks, NOCRED, 0);

	The second formal parameter of chkdq is declared to be a long. Since
	oip->i_ffs_blocks is u_int32_t, negating it may not give the expected
	result on any platform, and on an alpha, converting the result to a
	long will actually result in a positive argument passed to chkdq,
	independent of the value of oip->i_ffs_blocks (se code below).  The
	crash is caused by this positive argument in combination with
	NOCRED == ((struct ucred *)-1). chkdq assumes a positive second
	argument only to occur together with a valid cred-pointer.

	Not that I'm an ANSI-C expert, but I think the result of negating a
	u_int32_t is (allowed to be) implementation dependent even on
	platforms where (sizeof long) == sizeof(u_int32_t).

	Just to illustrate the conversion problem:

	#include <stdio.h>
	#include <stdlib.h>

	main()
	{
        	printf("%016lx\n",(long)-(unsigned int)1);
        	printf("%016lx\n",-(long)(unsigned int)1);
	}

	gives (on the alpha):

	00000000ffffffff
	ffffffffffffffff

>How-To-Repeat:
	Using the system... update's periodic activity usually triggers the
	crash before the system is completely up.
>Fix:
	For the alpha, changing the line (line 237 in
	/usr/src/sys/ufs/ffs/ffs_inode.c, rev. 1.10) to:

	(void) chkdq(oip, -(long)oip->i_ffs_blocks, NOCRED, 0);
	
	fix the problem. For platforms with sizeof(long) == sizeof(u_int32_t)
	this should work as long as oip->i_ffs_blocks < 2147483648.

	However, since di_blocks is used together with signed variables in
	other routines, changing di_blocks back to int32_t may be a better
	solution until all uses of di_blocks are examined. 

>Audit-Trail:
>Unformatted: