svn commit: r364402 - head/sys/kern
Shawn Webb
shawn.webb at hardenedbsd.org
Wed Aug 19 17:48:11 UTC 2020
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.
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20200819/4ba15350/attachment.sig>
More information about the svn-src-head
mailing list