Date: Sun, 09 Sep 2001 04:47:12 -0700 From: Dima Dorfman <dima@unixfreak.org> To: "Andrew R. Reiter" <arr@watson.org> Cc: Kris Kennaway <kris@obsecurity.org>, security@freebsd.org Subject: Re: netbsd vulnerabilities Message-ID: <20010909114717.80C903E28@bazooka.unixfreak.org> In-Reply-To: <Pine.NEB.3.96L.1010908063851.9148A-200000@fledge.watson.org>; from arr@watson.org on "Sat, 8 Sep 2001 06:43:49 -0400 (EDT)"
next in thread | previous in thread | raw e-mail | index | archive | help
"Andrew R. Reiter" <arr@watson.org> wrote:
> The attached code fixes the semop bug which is specified in the recent
> NetBSD security announcement. I'm not positive about hte naming scheme
> wanted by all in terms of: size_t vs. unsigned int vs. unsigned. I made
> it u_int b/c i saw in sysproto.h that there seemed to be more u_int's
> instead of size_t's :-) Great logic.
I think semop_args.nsops should be u_int (like you made it) because
that's how it's listed in syscalls.master.
> --- kern/sysv_sem.c.orig Sat Sep 8 03:11:21 2001
> +++ kern/sysv_sem.c Sat Sep 8 03:20:23 2001
> @@ -672,7 +672,7 @@
> struct semop_args {
> int semid;
> struct sembuf *sops;
> - int nsops;
> + u_int nsops;
> };
> #endif
>
> @@ -682,17 +682,18 @@
> register struct semop_args *uap;
> {
> int semid = uap->semid;
> - int nsops = uap->nsops;
> + u_int nsops = uap->nsops;
> struct sembuf sops[MAX_SOPS];
> register struct semid_ds *semaptr;
> register struct sembuf *sopptr;
> register struct sem *semptr;
> struct sem_undo *suptr = NULL;
> - int i, j, eval;
> + u_int i, j;
> + int eval;
What's the point of this change? i and j are used as indices into
sops[], and don't really need to be unsigned. Furthermore, I think
this change introduces an infinite loop bug. On line 1017 (r1.34), we
have:
for (j = i - 1; j >= 0; j--) {
if ((sops[j].sem_flg & SEM_UNDO) == 0)
continue;
adjval = sops[j].sem_op;
if (adjval == 0)
continue;
if (semundo_adjust(p, &suptr, semid,
sops[j].sem_num, adjval) != 0)
panic("semop - can't undo undos");
}
Since j is unsigned, the test in the for loop will always succeed, and
the only other way out of this loop is by way of panic(). That said,
I'm not sure if this loop can ever be entered as a result of the user
doing something (i.e., can't be "exploited" per se)--although I
haven't tried. It is only executed if semundo_adjust() fails, which,
as far as I can tell, can only happen if memory allocation fails.
Nevertheless, it isn't good to have a sure-thing infinite loop in the
kernel :-).
> --- sys/sem.h.orig Sat Sep 8 03:21:08 2001
> +++ sys/sem.h Sat Sep 8 03:21:27 2001
> @@ -101,7 +101,7 @@
> int semsys __P((int, ...));
> int semctl __P((int, int, int, ...));
> int semget __P((key_t, int, int));
> -int semop __P((int, struct sembuf *,unsigned));
> +int semop __P((int, struct sembuf *, u_int));
I don't see the point of this, either, except to break consistency
with the manual page. `u_int' is the same as `unsigned'.
The other changes look pretty good. Attached is the corresponding
patch to -current. If nobody sees anything wrong in about a day, I'll
commit this and MFC it after the RE's approval.
Kris, SO--objections?
Thanks.
Index: sysv_sem.c
===================================================================
RCS file: /ref/cvsf/src/sys/kern/sysv_sem.c,v
retrieving revision 1.34
diff -u -r1.34 sysv_sem.c
--- sysv_sem.c 31 Aug 2001 00:02:18 -0000 1.34
+++ sysv_sem.c 9 Sep 2001 11:28:32 -0000
@@ -781,7 +781,7 @@
struct semop_args {
int semid;
struct sembuf *sops;
- int nsops;
+ u_int nsops;
};
#endif
@@ -794,7 +794,7 @@
register struct semop_args *uap;
{
int semid = uap->semid;
- int nsops = uap->nsops;
+ u_int nsops = uap->nsops;
struct sembuf sops[MAX_SOPS];
register struct semid_ds *semaptr;
register struct sembuf *sopptr;
@@ -804,7 +804,7 @@
int do_wakeup, do_undos;
#ifdef SEM_DEBUG
- printf("call to semop(%d, 0x%x, %d)\n", semid, sops, nsops);
+ printf("call to semop(%d, 0x%x, %u)\n", semid, sops, nsops);
#endif
mtx_lock(&Giant);
@@ -840,7 +840,7 @@
if (nsops > MAX_SOPS) {
#ifdef SEM_DEBUG
- printf("too many sops (max=%d, nsops=%d)\n", MAX_SOPS, nsops);
+ printf("too many sops (max=%d, nsops=%u)\n", MAX_SOPS, nsops);
#endif
error = E2BIG;
goto done2;
@@ -848,7 +848,7 @@
if ((error = copyin(uap->sops, &sops, nsops * sizeof(sops[0]))) != 0) {
#ifdef SEM_DEBUG
- printf("error = %d from copyin(%08x, %08x, %d)\n", error,
+ printf("error = %d from copyin(%08x, %08x, %u)\n", error,
uap->sops, &sops, nsops * sizeof(sops[0]));
#endif
goto done2;
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-security" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010909114717.80C903E28>