Re: make_dev(9) perms for SCSI & SCSI RAID drivers in CURRENT.

On Sun, 8 Feb 2004, Andre Guibert de Bruet wrote:

> While studying the various FreeBSD SCSI and SCSI RAID drivers, I noticed
> that the file mode (perm mask) varies per driver. So far, I've come across
> 0600, 0640 and 0644. I can't really see why any of these drivers would
> have anything other than 0600, as it would require root access or at least
> write perm to do anything useful with the card.

All disk (data) devices should have mode 0640 and ownership root:operator
and all disk (control) devices should have mode 0600 and ownership root:wheel.
Distributed setting of ownerships and permissions gives many more bugs than
centralized setting in MAKEDEV.  Mode bugs in devfs start at its top level
(its directory has mode 555 although its owner can write to it except
possibly in the jailed case).

> Here's a quick illustration of what I'm refering to:
>
> aac	0640 (octal notation in code)
> amr	0600 (implemented as S_IRUSR | S_IWUSR)
> asr	0640 (octal notation in code)
> ciss	0600 (implemented as S_IRUSR | S_IWUSR)
> ida	0600 (implemented as S_IRUSR | S_IWUSR)
> iir	0644 (implemented as S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
> ips	0600 (implemented as S_IRUSR | S_IWUSR)
> isp	0600 (octal notation in code)
> mly	0600 (implemented as S_IRUSR | S_IWUSR)

Most of these actually create control devices, so mode 0600 is correct
and group operator is bogus, and mode 0640 is a potental security hole
especially with group operator.  Group operator is almost always used
of course.  The data devices are mostly created by the disk mini-layer
in RELENG_4 (except RELENG_4 doesn't really have devfs) and by GEOM in
-current.

> I've attached a set of patches that make the aac, asr, iir and isp drivers
> conform to S_IRUSR | S_IWUSR. I don't have all of these cards, but the
> drivers build on today's CURRENT and these changes are rather minor. As
> always, comments are welcome. :)

% Index: aac.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/dev/aac/aac.c,v
% retrieving revision 1.85
% diff -u -r1.85 aac.c
% --- aac.c	7 Feb 2004 17:40:37 -0000	1.85
% +++ aac.c	8 Feb 2004 08:09:48 -0000
% _at__at_ -51,6 +51,7 _at__at_
%  #include <sys/signalvar.h>
%  #include <sys/time.h>
%  #include <sys/eventhandler.h>
% +#include <sys/stat.h>
%
%  #include <machine/bus_memio.h>
%  #include <machine/bus.h>
% _at__at_ -271,7 +272,7 _at__at_
%  	 */
%  	unit = device_get_unit(sc->aac_dev);
%  	sc->aac_dev_t = make_dev(&aac_cdevsw, unit, UID_ROOT, GID_OPERATOR,
% -				 0640, "aac%d", unit);
% +				 S_IRUSR | S_IWUSR, "aac%d", unit);
%  	(void)make_dev_alias(sc->aac_dev_t, "afa%d", unit);
%  	(void)make_dev_alias(sc->aac_dev_t, "hpn%d", unit);
%  	sc->aac_dev_t->si_drv1 = sc;

This is the control device.  The change closes the security hole but leaves
a bogus group.  Correct attributes may be found in any (?) version of
MAKEDEV that supports aac:

%%%
aac*)
	unit=`expr $i : 'aac\(.*\)'`
	mknod aac$unit c 150 `unit2minor $unit`
	ln -fs aac$unit afa$unit
	ln -fs aac$unit hpn$unit
	;;
%%%

The default for MAKEDEV is mode 0600 ownership root:wheel, so secure
permissions and ownerships are automatic.  make_dev() should have
similar defaults, or macros for secure and other classes of attributes
should be used (corresponding to $secure_umask and $disk_umask in
MAKEDEV).  disk_umask=037 corresponds to mode 0640.

The change preserves style bugs (-ce instead of -ci4 indentation).

% Index: asr.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/dev/asr/asr.c,v
% retrieving revision 1.38
% diff -u -r1.38 asr.c
% --- asr.c	26 Sep 2003 15:56:42 -0000	1.38
% +++ asr.c	8 Feb 2004 07:59:18 -0000
% _at__at_ -3127,8 +3127,8 _at__at_
%  	/*
%  	 *	Generate the device node information
%  	 */
% -	(void)make_dev(&asr_cdevsw, unit, UID_ROOT, GID_OPERATOR, 0640,
% -	    "rasr%d", unit);
% +	(void)make_dev(&asr_cdevsw, unit, UID_ROOT, GID_OPERATOR,
% +	    S_IRUSR | S_IWUSR, "rasr%d", unit);
%  	ATTACH_RETURN(0);
%  } /* asr_attach */
%

Similarly, except asr is not in RELENG_4's MAKEDEV, the device is not so
clearly a control device 9aac.c has an explicit comment about this but aar.c
only mentions control devices in its in-file history, and asr still hasn't
caught up with the removal of the 'r' devices which occurred about 2
months before asr was imported.  asr seems to be a normal SCSI disk driver
so its disks are named da*.  Apparently its control devices is so little
used that is not missed.

% Index: iir_ctrl.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/dev/iir/iir_ctrl.c,v
% retrieving revision 1.11
% diff -u -r1.11 iir_ctrl.c
% --- iir_ctrl.c	26 Sep 2003 15:36:47 -0000	1.11
% +++ iir_ctrl.c	8 Feb 2004 07:56:45 -0000
% _at__at_ -103,12 +103,12 _at__at_
%
%  #ifdef SDEV_PER_HBA
%      dev = make_dev(&iir_cdevsw, hba2minor(unit), UID_ROOT, GID_OPERATOR,
% -                   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH, "iir%d", unit);
% +                   S_IRUSR | S_IWUSR, "iir%d", unit);
%  #else
%      if (sdev_made)
%          return (0);
%      dev = make_dev(&iir_cdevsw, 0, UID_ROOT, GID_OPERATOR,
% -                   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH, "iir");
% +                   S_IRUSR | S_IWUSR, "iir");
%      sdev_made = 1;
%  #endif
%      return (dev);

This is clearly a control device.  The change closes the security hole but
leaves a bogus group and preserves style bugs as in aac.c.  The control
device has never been in MAKEDEV, like asr but unlike aac.

% Index: isp_freebsd.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/dev/isp/isp_freebsd.c,v
% retrieving revision 1.96
% diff -u -r1.96 isp_freebsd.c
% --- isp_freebsd.c	7 Feb 2004 03:47:33 -0000	1.96
% +++ isp_freebsd.c	8 Feb 2004 08:12:40 -0000
% _at__at_ -35,6 +35,7 _at__at_
%  #include <sys/conf.h>
%  #include <sys/module.h>
%  #include <sys/ioccom.h>
% +#include <sys/stat.h>
%  #include <dev/isp/isp_ioctl.h>
%
%
% _at__at_ -211,7 +212,7 _at__at_
%  	 * Create device nodes
%  	 */
%  	(void) make_dev(&isp_cdevsw, device_get_unit(isp->isp_dev), UID_ROOT,
% -	    GID_OPERATOR, 0600, "%s", device_get_nameunit(isp->isp_dev));
% +	    GID_OPERATOR, S_IRUSR | S_IWUSR, "%s", device_get_nameunit(isp->isp_dev));
%
%  	if (isp->isp_role != ISP_ROLE_NONE) {
%  		isp->isp_state = ISP_RUNSTATE;

Seems to be like asr except it had no security hole or 'r' in the device name.
The change introduces a style bug (long line).

Bruce