From owner-freebsd-audit Tue May 22 20:29:30 2001 Delivered-To: freebsd-audit@freebsd.org Received: from bazooka.unixfreak.org (bazooka.unixfreak.org [63.198.170.138]) by hub.freebsd.org (Postfix) with ESMTP id 8DD1737B424 for ; Tue, 22 May 2001 20:29:24 -0700 (PDT) (envelope-from dima@unixfreak.org) Received: from spike.unixfreak.org (spike [63.198.170.139]) by bazooka.unixfreak.org (Postfix) with ESMTP id 3878C3E7D; Tue, 22 May 2001 20:29:24 -0700 (PDT) To: Thomas Moestl , audit@freebsd.org Subject: Re: Patch to remove setgid bit from ipcs(1) In-Reply-To: <20010523011938.A8824@crow.dom2ip.de>; from tmoestl@gmx.net on "Wed, 23 May 2001 01:19:38 +0200" Date: Tue, 22 May 2001 20:29:24 -0700 From: Dima Dorfman Message-Id: <20010523032924.3878C3E7D@bazooka.unixfreak.org> Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Thomas Moestl writes: > On Sun, 2001/05/20 at 16:17:51 -0700, Dima Dorfman wrote: > > Index: sys/kern/sysv_msg.c > > =================================================================== > > [...] > > +SYSCTL_DECL(_kern_ipc); > > +SYSCTL_STRUCT(_kern_ipc, OID_AUTO, msginfo, CTLFLAG_RD, &msginfo, msginfo, > > + "System V message info"); > > +SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLFLAG_ANYBODY | CTLFLAG_RD, > > + NULL, 0, sysctl_msqids, "", "Message queue IDs"); > > CTLFLAG_ANYBODY is pointless here since this variable is not > writable. Reading is allowed for all users (unless the handler > implements special restrictions). Indeed. I'll get rid of that. > > > Index: sys/kern/sysv_sem.c > > =================================================================== > > RCS file: /stl/src/FreeBSD/src/sys/kern/sysv_sem.c,v > > retrieving revision 1.32 > > diff -u -r1.32 sysv_sem.c > > --- sys/kern/sysv_sem.c 2001/02/21 06:39:54 1.32 > > +++ sys/kern/sysv_sem.c 2001/05/20 22:54:55 > > @@ -28,6 +28,7 @@ > > static int sysvsem_modload __P((struct module *, int, void *)); > > static int semunload __P((void)); > > static void semexit_myhook __P((struct proc *p)); > > +static int sysctl_sema __P((SYSCTL_HANDLER_ARGS)); > > > > #ifndef _SYS_SYSPROTO_H_ > > struct __semctl_args; > > @@ -148,6 +149,9 @@ > > SYSCTL_INT(_kern_ipc, OID_AUTO, semusz, CTLFLAG_RD, &seminfo.semusz, 0, "" > ); > > SYSCTL_INT(_kern_ipc, OID_AUTO, semvmx, CTLFLAG_RW, &seminfo.semvmx, 0, "" > ); > > SYSCTL_INT(_kern_ipc, OID_AUTO, semaem, CTLFLAG_RW, &seminfo.semaem, 0, "" > ); > > +SYSCTL_STRUCT(_kern_ipc, OID_AUTO, seminfo, CTLFLAG_RD, &seminfo, seminfo, > ""); > > Hmm, it seems to me that we export all members of this structure, so > why export it again as a whole? While it might be better to pack things > into a structure (which may however introduce problems when the > structure changes), I'm not sure whether should really export this > more than once just because of that. > This also seems to apply to shared memory part. It's a compromise between exorting the entire structure or complicating the userland part to do sysctl on all the different fields and construct the structure itself. This was the simpler approach, which is why I chose it. Do you think it's worth complicating the kget() routine instead? > > > Index: usr.bin/ipcs/ipcs.c > > =================================================================== > > [...] > > static struct nlist symbols[] = { > > @@ -63,16 +67,14 @@ > > #define X_SEMA 0 > > {"_seminfo"}, > > #define X_SEMINFO 1 > > - {"_semu"}, > > -#define X_SEMU 2 > > {"_msginfo"}, > > -#define X_MSGINFO 3 > > +#define X_MSGINFO 2 > > {"_msqids"}, > > -#define X_MSQIDS 4 > > +#define X_MSQIDS 3 > > {"_shminfo"}, > > -#define X_SHMINFO 5 > > +#define X_SHMINFO 4 > > {"_shmsegs"}, > > -#define X_SHMSEGS 6 > > +#define X_SHMSEGS 5 > > {NULL} > > }; > > I think you can safely remove the underscores, they are not needed any > more. Fair enough. I tried to leave alone as many things as I could, but I guess this is the best time to do that. > > > void > > +kget(idx, addr, size) > > + int idx; > > + void *addr; > > + size_t size; > > +{ > > [...] > > + if (rv != tsiz) > > + errx(1, "%s: %s", symn, kvm_geterr(kd)); > > + if (kvm_read(kd, kaddr, addr, size) != size) > > + errx(1, "%s: %s", symn, kvm_geterr(kd)); > > I think the error message is redundant here, since you have passed an > error string to kvm_open (which causes kvm library routines to print > errors to stderr). Maybe printing the symbol name and just exiting > should be sufficient. The error buffer seems not even to be filled > when that string is set. This is indeed the documented behavior, but this isn't how the libkvm code behaves. If I stick a "kaddr = 0;" right above that block, I get the expected output: dima@spike% sudo ./ipcs -y ipcs: msginfo: kvm_read: Bad address I can confirm that it is indeed the errx that is printing that. Am I missing something, or is libkvm doing the wrong thing here? > > > + } else { > > + tsiz = size; > > + if (sysctlbyname(sym2sysctl[idx], addr, &tsiz, NULL, 0) > > + == -1) > > + err(1, "sysctlbyname: %s", sym2sysctl[idx]); > > + } > > +} > > I think you should check that the size that we read is actually the > one we expected (if (tsiz != size) barf();). Good idea. I'll do that. I'll send an updated patch when I/we figure out what to with regards to the sysctl's and the libkvm stuff. Thanks for the input, Dima Dorfman dima@unixfreak.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message