From owner-svn-src-head@freebsd.org Wed Aug 19 17:54:51 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 703943C5A6C for ; Wed, 19 Aug 2020 17:54:51 +0000 (UTC) (envelope-from shawn.webb@hardenedbsd.org) Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BWwQy45Glz4MNs for ; Wed, 19 Aug 2020 17:54:50 +0000 (UTC) (envelope-from shawn.webb@hardenedbsd.org) Received: by mail-qk1-x72b.google.com with SMTP id p4so22430613qkf.0 for ; Wed, 19 Aug 2020 10:54:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hardenedbsd.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=IYngxZZxZQimYzRyfCUC2hLlW+cspvmhKY9GOoV0Wck=; b=EaV3TKKq28hLg1Vk6hrJxwP3rhnu7z9Tu/Iuw6Tiv/mpZsQsM+papghGyrj0UJ7qz5 rTveIrLGyf8appFOYUp8C5DSWGIkdKSDahUU226kOa4b0mV+yzJmR14COl34sjwG9JAW J3Ztxf50bda+mJCS1fviwsQlxdsUKhQx1lniOb4dAWPUTiRtuQNPHnDA1ff8HaAfsVBP 88OFCIJGN7T1itFrlNE47WbVbMVL7Ae9iohZLTkdaDjRNtswr5qfVxTgyyoXgSpNKqGQ j3TAnandyJQOGbcFPNKf3rLqvribq2Id1pH7ooKgTHB8scXRzFIdxGqnQEmyYtwyczwB OUXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=IYngxZZxZQimYzRyfCUC2hLlW+cspvmhKY9GOoV0Wck=; b=YLSZ0qeOc1EZFM1wkP+Ywqn63jIIkAq7vba0Wbx61v2aLbGNMGdCYNnGbf/PN84GO4 mI5co8rv1gW/XrPrkli50lQRuV222qS43rgB+bVQawtFWn+kFqa7BsX9PP/M7AjF9Q3z vCyjIMH21PZvOw82xEOjIdv/gt6gPylTENlrQHkoCw4dMxh87NJoF0TQGC7Lkxm1za3H yKY2Wm39PTz2158yhyAHEq2AsPUU1GyZTKia4tMAzLZgznjAn7lHfRH6Q1CU4yEtqtE6 0jFmFiqOV6EWpuIUkRPbqpQUZRCzn62gUURfrugrwUL8OdOJS9pIKjjshvENWB0DGFHs M73w== X-Gm-Message-State: AOAM530OBhx7Dve9RL6sQfk3aQRtOCOdqDKJgriKPVUsWNErRyI0BCDR htqe+TB1z6G30MsdR5QAYBO/Dw== X-Google-Smtp-Source: ABdhPJyPJJ05b8S2FLYtBRkEXIZZHax1tugHhfUx1mZ60xWloGg72UvKMQqyG41JHc3y8gVmchkhuQ== X-Received: by 2002:a05:620a:22e9:: with SMTP id p9mr23772175qki.105.1597859689238; Wed, 19 Aug 2020 10:54:49 -0700 (PDT) Received: from mutt-hbsd (pool-100-16-231-224.bltmmd.fios.verizon.net. [100.16.231.224]) by smtp.gmail.com with ESMTPSA id t1sm25071945qkt.119.2020.08.19.10.54.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Aug 2020 10:54:48 -0700 (PDT) Date: Wed, 19 Aug 2020 13:54:47 -0400 From: Shawn Webb To: Warner Losh Cc: Warner Losh , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r364402 - head/sys/kern Message-ID: <20200819175447.pjoa2pmusdbql22q@mutt-hbsd> X-Operating-System: FreeBSD mutt-hbsd 13.0-CURRENT-HBSD FreeBSD 13.0-CURRENT-HBSD X-PGP-Key: http://pgp.mit.edu/pks/lookup?op=vindex&search=0xFF2E67A277F8E1FA References: <202008191710.07JHA5Rk008764@repo.freebsd.org> <20200819172613.vdyutsn6a4w5fbqr@mutt-hbsd> <20200819174808.ts3i72vzqsx3sq5o@mutt-hbsd> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hfbjzt6hwnwusfdh" Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 4BWwQy45Glz4MNs X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=hardenedbsd.org header.s=google header.b=EaV3TKKq; dmarc=none; spf=pass (mx1.freebsd.org: domain of shawn.webb@hardenedbsd.org designates 2607:f8b0:4864:20::72b as permitted sender) smtp.mailfrom=shawn.webb@hardenedbsd.org X-Spamd-Result: default: False [-2.34 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; R_DKIM_ALLOW(-0.20)[hardenedbsd.org:s=google]; FROM_HAS_DN(0.00)[]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36:c]; MIME_GOOD(-0.20)[multipart/signed,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[hardenedbsd.org]; RCPT_COUNT_FIVE(0.00)[5]; RCVD_COUNT_THREE(0.00)[3]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[hardenedbsd.org:+]; NEURAL_HAM_SHORT(-0.24)[-0.238]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::72b:from]; SIGNED_PGP(-2.00)[]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; MID_RHS_NOT_FQDN(0.50)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCVD_TLS_ALL(0.00)[]; MAILMAN_DEST(0.00)[svn-src-head]; RECEIVED_SPAMHAUS_PBL(0.00)[100.16.231.224:received] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Aug 2020 17:54:51 -0000 --hfbjzt6hwnwusfdh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 19, 2020 at 11:51:10AM -0600, Warner Losh wrote: > On Wed, Aug 19, 2020 at 11:48 AM Shawn Webb > wrote: >=20 > > On Wed, Aug 19, 2020 at 11:44:42AM -0600, Warner Losh wrote: > > > On Wed, Aug 19, 2020 at 11:26 AM Shawn Webb > > > 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 > > > > > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > > > --- 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 > > > > > #include > > > > > #include > > > > > +#include > > > > > #include > > > > > #include > > > > > #include > > > > > @@ -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, bo= ol > > > > 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 !=3D 0) > > > > > goto end; > > > > > > > > > > + dev_vfs_event("REMOUNT", mp, true); > > > > > if (mp->mnt_opt !=3D NULL) > > > > > vfs_freeopts(mp->mnt_opt); > > > > > mp->mnt_opt =3D mp->mnt_optnew; > > > > > @@ -1839,6 +1844,7 @@ dounmount(struct mount *mp, int flags, stru= ct > > > > 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 !=3D NULL) { > > > > > coveredvp->v_mountedhere =3D NULL; > > > > > vn_seqc_write_end(coveredvp); > > > > > @@ -2425,4 +2431,72 @@ kernel_vmount(int flags, ...) > > > > > > > > > > error =3D kernel_mount(ma, flags); > > > > > return (error); > > > > > +} > > > > > + > > > > > +/* Map from mount options to printable formats. */ > > > > > +static struct mntoptnames optnames[] =3D { > > > > > + MNTOPT_NAMES > > > > > +}; > > > > > + > > > > > +static void > > > > > +dev_vfs_event_mntopt(struct sbuf *sb, const char *what, struct > > > > vfsoptlist *opts) > > > > > +{ > > > > > + struct vfsopt *opt; > > > > > + > > > > > + if (opts =3D=3D NULL || TAILQ_EMPTY(opts)) > > > > > + return; > > > > > + sbuf_printf(sb, " %s=3D\"", what); > > > > > + TAILQ_FOREACH(opt, opts, link) { > > > > > + if (opt->name[0] =3D=3D '\0' || (opt->len > 0 && *(= char > > > > *)opt->value =3D=3D '\0')) > > > > > + continue; > > > > > + devctl_safe_quote_sb(sb, opt->name); > > > > > + if (opt->len > 0) { > > > > > + sbuf_putc(sb, '=3D'); > > > > > + 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 =3D &mp->mnt_stat; > > > > > + char *buf; > > > > > + > > > > > + buf =3D malloc(DEVCTL_LEN, M_MOUNT, M_WAITOK); > > > > > + if (buf =3D=3D NULL) > > > > > + return; > > > > > > > > buf can't be NULL. > > > > > > > > > > The bug here is that M_NOWAIT should have been specified in the mallo= c. > > > > > > > > > > > + sbuf_new(&sb, buf, DEVCTL_LEN, SBUF_FIXEDLEN); > > > > > + sbuf_cpy(&sb, "mount-point=3D\""); > > > > > + devctl_safe_quote_sb(&sb, sfp->f_mntonname); > > > > > + sbuf_cat(&sb, "\" mount-dev=3D\""); > > > > > + devctl_safe_quote_sb(&sb, sfp->f_mntfromname); > > > > > + sbuf_cat(&sb, "\" mount-type=3D\""); > > > > > + devctl_safe_quote_sb(&sb, sfp->f_fstypename); > > > > > + sbuf_cat(&sb, "\" fsid=3D0x"); > > > > > + cp =3D (const uint8_t *)&sfp->f_fsid.val[0]; > > > > > + for (int i =3D 0; i < sizeof(sfp->f_fsid); i++) > > > > > + sbuf_printf(&sb, "%02x", cp[i]); > > > > > + sbuf_printf(&sb, " owner=3D%u flags=3D\"", sfp->f_owner); > > > > > + for (fp =3D optnames; fp->o_opt !=3D 0; fp++) { > > > > > + if ((mp->mnt_flag & fp->o_opt) !=3D 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 overflo= w. > > > > 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 sho= uld > > > just not send it if there's an overflow... There almost certainly wo= n'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. > > >=20 > Right, which is why we should check. >=20 > See https://reviews.freebsd.org/D26122 for the proper tweak (imho). Why spend so much time in the kernel just to fail at the end? Why not fail fast, checking for sbuf_* returning -1 along the way? --=20 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/Sha= wn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc --hfbjzt6hwnwusfdh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEA6TL67gupaZ9nzhT/y5nonf44foFAl89Z2cACgkQ/y5nonf4 4fow2Q//VfcnugxVzIVaad043nv5oXZNsiyTM28qHVU5Oq9BHw4Pkoh5EyLn7xrn 8odXES1eO+9cOgS7UF7XwQEWLY27i9X1cZ1cPYPlt7Av9TytNW9V1E2YgseJdSvA tM06D4H35l3b2rjdxb625/mIK3vx5qic/sl1xhrO4gipkEZseL/qS2C1h0/6cX7b AfuCbdkktXKECzCJqAYLtVUBgpY/VGYTS2FYd/gmnH5a37ESvfy9s4B8WmOg6EGo mGHx9D0DEsGBh7V8Y5cNno3EnNNH6pj2bRhw+7KfqUs8V9UbR/mTYl+611l2U5/i Y+wYyyo1kskkHRQo9Bb5KN0/+feobekzHuXcb2gtAhka/UrSp07A1TqJOmn/YA/z ohJzWfXJzqPOx5qn0Kkikfky30jryBpxrO90Qv1aE/wt4lKMXn+uKMh/1dQ34rxH pWAFaNOGfmjeayEisgJ4yN8mJLKQ4Isc9mWLVOJTt+hR1vKya8HKQJ9QDzo8T0x3 XfICPRVKPA+N6Hw3Of7BhaTxyduiZqaRW09X9F2QBbKKdiTG0wCjBq8H0KzSh4P0 x2f6onSJLPbfgyE7FmRYhAIwBEYl/CfnbMx4xdnsnAg1Im/zc76vsATFqxWT3Lis 11+UqgseLwhqbuc/6NyMGMbLx9DwADGDPo/RjgC0aH5T8mWh5v4= =Hbtr -----END PGP SIGNATURE----- --hfbjzt6hwnwusfdh--