svn commit: r364402 - head/sys/kern
Warner Losh
imp at bsdimp.com
Wed Aug 19 17:51:22 UTC 2020
On Wed, Aug 19, 2020 at 11:48 AM Shawn Webb <shawn.webb at hardenedbsd.org>
wrote:
> On Wed, Aug 19, 2020 at 11:44:42AM -0600, Warner Losh wrote:
> > On Wed, Aug 19, 2020 at 11:26 AM Shawn Webb <shawn.webb at hardenedbsd.org>
> > wrote:
> >
> > > On Wed, Aug 19, 2020 at 05:10:05PM +0000, Warner Losh wrote:
> > > > Author: imp
> > > > Date: Wed Aug 19 17:10:04 2020
> > > > New Revision: 364402
> > > > URL: https://svnweb.freebsd.org/changeset/base/364402
> > > >
> > > > Log:
> > > > Add VFS FS events for mount and unmount to devctl/devd
> > > >
> > > > Report when a filesystem is mounted, remounted or unmounted via
> devd,
> > > along with
> > > > details about the mount point and mount options.
> > > >
> > > > Discussed with: kib@
> > > > Reviewed by: kirk@ (prior version)
> > > > Sponsored by: Netflix
> > > > Diffential Revision: https://reviews.freebsd.org/D25969
> > > >
> > > > Modified:
> > > > head/sys/kern/vfs_mount.c
> > > >
> > > > Modified: head/sys/kern/vfs_mount.c
> > > >
> > >
> ==============================================================================
> > > > --- head/sys/kern/vfs_mount.c Wed Aug 19 17:09:58 2020
> (r364401)
> > > > +++ head/sys/kern/vfs_mount.c Wed Aug 19 17:10:04 2020
> (r364402)
> > > > @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");
> > > > #include <sys/param.h>
> > > > #include <sys/conf.h>
> > > > #include <sys/smp.h>
> > > > +#include <sys/bus.h>
> > > > #include <sys/eventhandler.h>
> > > > #include <sys/fcntl.h>
> > > > #include <sys/jail.h>
> > > > @@ -101,6 +102,8 @@ MTX_SYSINIT(mountlist, &mountlist_mtx,
> "mountlist",
> > > MT
> > > > EVENTHANDLER_LIST_DEFINE(vfs_mounted);
> > > > EVENTHANDLER_LIST_DEFINE(vfs_unmounted);
> > > >
> > > > +static void dev_vfs_event(const char *type, struct mount *mp, bool
> > > donew);
> > > > +
> > > > /*
> > > > * Global opts, taken by all filesystems
> > > > */
> > > > @@ -1020,6 +1023,7 @@ vfs_domount_first(
> > > > VOP_UNLOCK(vp);
> > > > EVENTHANDLER_DIRECT_INVOKE(vfs_mounted, mp, newdp, td);
> > > > VOP_UNLOCK(newdp);
> > > > + dev_vfs_event("MOUNT", mp, false);
> > > > mountcheckdirs(vp, newdp);
> > > > vn_seqc_write_end(vp);
> > > > vn_seqc_write_end(newdp);
> > > > @@ -1221,6 +1225,7 @@ vfs_domount_update(
> > > > if (error != 0)
> > > > goto end;
> > > >
> > > > + dev_vfs_event("REMOUNT", mp, true);
> > > > if (mp->mnt_opt != NULL)
> > > > vfs_freeopts(mp->mnt_opt);
> > > > mp->mnt_opt = mp->mnt_optnew;
> > > > @@ -1839,6 +1844,7 @@ dounmount(struct mount *mp, int flags, struct
> > > thread *
> > > > TAILQ_REMOVE(&mountlist, mp, mnt_list);
> > > > mtx_unlock(&mountlist_mtx);
> > > > EVENTHANDLER_DIRECT_INVOKE(vfs_unmounted, mp, td);
> > > > + dev_vfs_event("UNMOUNT", mp, false);
> > > > if (coveredvp != NULL) {
> > > > coveredvp->v_mountedhere = NULL;
> > > > vn_seqc_write_end(coveredvp);
> > > > @@ -2425,4 +2431,72 @@ kernel_vmount(int flags, ...)
> > > >
> > > > error = kernel_mount(ma, flags);
> > > > return (error);
> > > > +}
> > > > +
> > > > +/* Map from mount options to printable formats. */
> > > > +static struct mntoptnames optnames[] = {
> > > > + MNTOPT_NAMES
> > > > +};
> > > > +
> > > > +static void
> > > > +dev_vfs_event_mntopt(struct sbuf *sb, const char *what, struct
> > > vfsoptlist *opts)
> > > > +{
> > > > + struct vfsopt *opt;
> > > > +
> > > > + if (opts == NULL || TAILQ_EMPTY(opts))
> > > > + return;
> > > > + sbuf_printf(sb, " %s=\"", what);
> > > > + TAILQ_FOREACH(opt, opts, link) {
> > > > + if (opt->name[0] == '\0' || (opt->len > 0 && *(char
> > > *)opt->value == '\0'))
> > > > + continue;
> > > > + devctl_safe_quote_sb(sb, opt->name);
> > > > + if (opt->len > 0) {
> > > > + sbuf_putc(sb, '=');
> > > > + devctl_safe_quote_sb(sb, opt->value);
> > > > + }
> > > > + sbuf_putc(sb, ';');
> > > > + }
> > > > + sbuf_putc(sb, '"');
> > > > +}
> > > > +
> > > > +#define DEVCTL_LEN 1024
> > > > +static void
> > > > +dev_vfs_event(const char *type, struct mount *mp, bool donew)
> > > > +{
> > > > + const uint8_t *cp;
> > > > + struct mntoptnames *fp;
> > > > + struct sbuf sb;
> > > > + struct statfs *sfp = &mp->mnt_stat;
> > > > + char *buf;
> > > > +
> > > > + buf = malloc(DEVCTL_LEN, M_MOUNT, M_WAITOK);
> > > > + if (buf == NULL)
> > > > + return;
> > >
> > > buf can't be NULL.
> > >
> >
> > The bug here is that M_NOWAIT should have been specified in the malloc.
> >
> >
> > > > + sbuf_new(&sb, buf, DEVCTL_LEN, SBUF_FIXEDLEN);
> > > > + sbuf_cpy(&sb, "mount-point=\"");
> > > > + devctl_safe_quote_sb(&sb, sfp->f_mntonname);
> > > > + sbuf_cat(&sb, "\" mount-dev=\"");
> > > > + devctl_safe_quote_sb(&sb, sfp->f_mntfromname);
> > > > + sbuf_cat(&sb, "\" mount-type=\"");
> > > > + devctl_safe_quote_sb(&sb, sfp->f_fstypename);
> > > > + sbuf_cat(&sb, "\" fsid=0x");
> > > > + cp = (const uint8_t *)&sfp->f_fsid.val[0];
> > > > + for (int i = 0; i < sizeof(sfp->f_fsid); i++)
> > > > + sbuf_printf(&sb, "%02x", cp[i]);
> > > > + sbuf_printf(&sb, " owner=%u flags=\"", sfp->f_owner);
> > > > + for (fp = optnames; fp->o_opt != 0; fp++) {
> > > > + if ((mp->mnt_flag & fp->o_opt) != 0) {
> > > > + sbuf_cat(&sb, fp->o_name);
> > > > + sbuf_putc(&sb, ';');
> > > > + }
> > > > + }
> > > > + sbuf_putc(&sb, '"');
> > > > + dev_vfs_event_mntopt(&sb, "opt", mp->mnt_opt);
> > > > + if (donew)
> > > > + dev_vfs_event_mntopt(&sb, "optnew", mp->mnt_optnew);
> > > > + sbuf_finish(&sb);
> > > > +
> > > > + devctl_notify("VFS", "FS", type, sbuf_data(&sb));
> > > > + sbuf_delete(&sb);
> > > > + free(buf, M_MOUNT);
> > > > }
> > >
> > > I don't really see much attention paid to checking for sbuf overflow.
> > > Could that cause issues, especially in case of impartial quotation
> > > termination? Could not performing those checks have security
> > > implications? Would performing those checks adhere to good code
> > > development practices?
> > >
> >
> > sbuf doesn't overflow. It is safe from that perspective. The code should
> > just not send it if there's an overflow... There almost certainly won't
> be
> > one in practice given the buffer size, though.
>
> You're right that sbuf can't overflow. However, assuming that it
> hasn't reached its fixed size specified above and continuing on as if
> there's still space left could lead to... interesting behavior.
>
Right, which is why we should check.
See https://reviews.freebsd.org/D26122 for the proper tweak (imho).
Warner
> --
> Shawn Webb
> Cofounder / Security Engineer
> HardenedBSD
>
> GPG Key ID: 0xFF2E67A277F8E1FA
> GPG Key Fingerprint: D206 BB45 15E0 9C49 0CF9 3633 C85B 0AF8 AB23 0FB2
>
> https://git-01.md.hardenedbsd.org/HardenedBSD/pubkeys/src/branch/master/Shawn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc
>
More information about the svn-src-head
mailing list