video(1) pledge (& updated kernel diff)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

video(1) pledge (& updated kernel diff)

Landry Breuil-5
Hi,

so i've updated my 'video' class for pledge and also did an initial
naive pledging of xenocara/app/video:

the kernel diff is the same subset as the diff from some months ago,
except that it adds:
- VIDIOC_QUERYCTRL/VIDIOC_G_CTRL/VIDIOC_S_CTRL: those 3 are used by
  video(1) to set gamma/contrast/etc controls on the device on the fly
(via A/a, B/b, C/c - etc when running)
- VIDIOC_TRY_FMT, used by
  https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc#418
since https://bugzilla.mozilla.org/show_bug.cgi?id=1376873 (yes, i
checked with ktrace, all the other ioctls are still used)

with that kernel diff, i'm able to record video in firefox with the
video pledge added to the main process.

as for the video(1) diff:
- fixed a typo
- renamed err to errs to avoid shadowing err()

and i've identified 4 main modes:
- -o needs to write to the fs (so wpath cpath) and read from the device
  (so video)
- -O needs the same subset and dns unix in addition, as it talks to Xv
- -i only needs rpath & dns/unix for xv, no access to the video device
  needed
- no option needs dns unix for xv, video for ioctls, and wpath as the
  video device is open with O_RDWR.

tested those 4 modes without aborts so far, video -q exits before
reaching pledge. Not sure if those pledge calls are at the best spot,
but that's a start.

I know video(1) sucks and doesnt work directly with Xv with most modern
builtin devices on laptops as they often dont support the few encodings
supported by video(1) for Xv, but with my old-school cameras lying
around it works fine.

Landry

video_1_pledge.diff (3K) Download Attachment
kern_pledge_video.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: video(1) pledge (& updated kernel diff)

Sebastien Marie-3
On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> Hi,
>
> so i've updated my 'video' class for pledge and also did an initial
> naive pledging of xenocara/app/video:

just a small note: video(1) is the sole customer for ioctl(VIDIOC_*) in
base. other potential customers are in ports.

personally, with such promise, I would be interested in pledging a SIP
program (like telephony/baresip, but I didn't check if this program is
designed well enough for be pledgeable). I will look at it in order to
have a third program to check the relevance of the ioctls.

> as for the video(1) diff:
> - fixed a typo
> - renamed err to errs to avoid shadowing err()

I would separate the addition of pledge(2) and unrelated fixes.
 

> and i've identified 4 main modes:
> - -o needs to write to the fs (so wpath cpath) and read from the device
>   (so video)
> - -O needs the same subset and dns unix in addition, as it talks to Xv
> - -i only needs rpath & dns/unix for xv, no access to the video device
>   needed
> - no option needs dns unix for xv, video for ioctls, and wpath as the
>   video device is open with O_RDWR.
>
> tested those 4 modes without aborts so far, video -q exits before
> reaching pledge. Not sure if those pledge calls are at the best spot,
> but that's a start.
>
> I know video(1) sucks and doesnt work directly with Xv with most modern
> builtin devices on laptops as they often dont support the few encodings
> supported by video(1) for Xv, but with my old-school cameras lying
> around it works fine.

I have old enough stuff to playing with video(1), so I will test it :)

> Index: video.c
> ===================================================================
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c 9 Apr 2018 18:16:44 -0000 1.25
> +++ video.c 28 Dec 2018 20:22:36 -0000
> @@ -1963,6 +1963,22 @@
>   if (vid.mode & M_QUERY) {
>   dev_dump_query(&vid);
>   cleanup(&vid, 0);
> + }
> +
> + if (vid.mode & M_OUT_FILE) {
> + if(vid.mode & M_OUT_XV) {
> + if (pledge("stdio rpath wpath cpath dns unix video", NULL) == -1)
> + err(1, "pledge");
> + } else {
> + if (pledge("stdio rpath wpath cpath video", NULL) == -1)
> + err(1, "pledge");
> + }
> + } else if (vid.mode & M_IN_FILE) {
> + if (pledge("stdio rpath dns unix", NULL) == -1)
> + err(1, "pledge");
> + } else {
> + if (pledge("stdio rpath wpath dns unix video", NULL) == -1)
> + err(1, "pledge");
>   }
>  
>   if (vid.fps == 0)

I think you pledged too early.

"dns unix" are here for XOpenDisplay(), but if the display is remote,
"inet" could be needed too. Even if Xv isn't possible on remote display,
XOpenDisplay() will be killed by pledge.

So ideally, pledge(2) should occurs later.


> Index: kern/kern_pledge.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.245
> diff -u -r1.245 kern_pledge.c
> --- kern/kern_pledge.c 17 Nov 2018 23:10:08 -0000 1.245
> +++ kern/kern_pledge.c 28 Dec 2018 20:23:02 -0000
> @@ -43,6 +43,7 @@
>  #include <sys/dkio.h>
>  #include <sys/mtio.h>
>  #include <sys/audioio.h>
> +#include <sys/videoio.h>
>  #include <net/bpf.h>
>  #include <net/route.h>
>  #include <net/if.h>
> @@ -396,6 +397,7 @@
>   { "tty", PLEDGE_TTY },
>   { "unix", PLEDGE_UNIX },
>   { "unveil", PLEDGE_UNVEIL },
> + { "video", PLEDGE_VIDEO },
>   { "vminfo", PLEDGE_VMINFO },
>   { "vmm", PLEDGE_VMM },
>   { "wpath", PLEDGE_WPATH },
> @@ -1158,6 +1160,34 @@
>   break;
>   }
>   }
> +
> + if ((p->p_p->ps_pledge & PLEDGE_VIDEO)) {
> + switch (com) {
> + case VIDIOC_QUERYCAP:
> + case VIDIOC_TRY_FMT:
> + case VIDIOC_ENUM_FMT:
> + case VIDIOC_S_FMT:
> + case VIDIOC_QUERYCTRL:
> + case VIDIOC_G_CTRL:
> + case VIDIOC_S_CTRL:
> + case VIDIOC_G_PARM:
> + case VIDIOC_S_PARM:
> + case VIDIOC_REQBUFS:
> + case VIDIOC_QBUF:
> + case VIDIOC_DQBUF:
> + case VIDIOC_QUERYBUF:
> + case VIDIOC_STREAMON:
> + case VIDIOC_STREAMOFF:
> + case VIDIOC_ENUM_FRAMESIZES:
> + case VIDIOC_ENUM_FRAMEINTERVALS:

I didn't check them for now. I will do it later.

Please note that I will take in consideration all ioctls provided by
video(4), and not only the one used by video(1) or firefox. The purpose
is to have usable "video" promise, without been widely opened neither
too restricted. It is why we should considere several programs at once
when designing the promise, and look at kernel code path to have the
attack surface opened by the promise.

> + if (fp->f_type == DTYPE_VNODE &&
> +    vp->v_type == VCHR &&
> +    cdevsw[major(vp->v_rdev)].d_open == videoopen)
> + return (0);
> + break;
> + }
> + }
> +
>  
>  #if NPF > 0
>   if ((p->p_p->ps_pledge & PLEDGE_PF)) {
> Index: sys/pledge.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/pledge.h,v
> retrieving revision 1.38
> diff -u -r1.38 pledge.h
> --- sys/pledge.h 11 Aug 2018 16:16:07 -0000 1.38
> +++ sys/pledge.h 28 Dec 2018 20:23:02 -0000
> @@ -62,6 +62,7 @@
>  #define PLEDGE_ERROR 0x0000000400000000ULL /* ENOSYS instead of kill */
>  #define PLEDGE_WROUTE 0x0000000800000000ULL /* interface address ioctls */
>  #define PLEDGE_UNVEIL 0x0000001000000000ULL /* allow unveil() */
> +#define PLEDGE_VIDEO 0x0000002008000000ULL /* video ioctls */

I suspect a copy/paste error: PLEDGE_VIDEO contains 2 bits sets (there is a '8' after the '2').
and space vs tab between the name and the value.
 

>  /*
>   * Bits outside PLEDGE_USERSET are used by the kernel itself
> @@ -111,6 +112,7 @@
>   { PLEDGE_ERROR, "error" },
>   { PLEDGE_WROUTE, "wroute" },
>   { PLEDGE_UNVEIL, "unveil" },
> + { PLEDGE_VIDEO, "video" },
>   { 0, NULL },
>  };
>  #endif

Thanks for the recall.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: video(1) pledge (& updated kernel diff)

Landry Breuil-5
On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:

> On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> > Hi,
> >
> > so i've updated my 'video' class for pledge and also did an initial
> > naive pledging of xenocara/app/video:
>
> just a small note: video(1) is the sole customer for ioctl(VIDIOC_*) in
> base. other potential customers are in ports.
>
> personally, with such promise, I would be interested in pledging a SIP
> program (like telephony/baresip, but I didn't check if this program is
> designed well enough for be pledgeable). I will look at it in order to
> have a third program to check the relevance of the ioctls.
>
> > as for the video(1) diff:
> > - fixed a typo
> > - renamed err to errs to avoid shadowing err()
>
> I would separate the addition of pledge(2) and unrelated fixes.
Right, two separated diffs attached for this.

> I think you pledged too early.
>
> "dns unix" are here for XOpenDisplay(), but if the display is remote,
> "inet" could be needed too. Even if Xv isn't possible on remote display,
> XOpenDisplay() will be killed by pledge.
>
> So ideally, pledge(2) should occurs later.

Yup, after some discussions, moving the pledge calls after setup()
(which does the file & xv opening) allows to only need stdio in the -i
case, and 'stdio rpath video' in the others. i thought rpath could be
dropped but in my testing with video -vvv -O i had a crash upon some
keys where x tried to open /usr/X11R6/share/X11/locale/locale.alias..

in addition we can also pledge the video -q case which needs
stdio/rpath/wpath/video.


> > +#define PLEDGE_VIDEO 0x0000002008000000ULL /* video ioctls */
>
> I suspect a copy/paste error: PLEDGE_VIDEO contains 2 bits sets (there is a '8' after the '2').
> and space vs tab between the name and the value.

Yes that was a typo, i think when i originally started this the next
available define was 80000000ULL :)

Landry

video_miscfixes.diff (2K) Download Attachment
video_pledge.2.diff (769 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: video(1) pledge (& updated kernel diff)

Sebastien Marie-3
On Sun, Dec 30, 2018 at 10:58:58AM +0100, Landry Breuil wrote:
> On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:
> > On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> >
> > I would separate the addition of pledge(2) and unrelated fixes.
>
> Right, two separated diffs attached for this.
>

the err->errs diff to avoid shadowing is ok semarie@

mherrb@, any objections to it ?

Thanks
--
Sebastien Marie

> Index: video.c
> ===================================================================
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c 9 Apr 2018 18:16:44 -0000 1.25
> +++ video.c 30 Dec 2018 09:39:21 -0000
> @@ -1854,7 +1854,7 @@
>   struct xdsp *x = &vid.xdsp;
>   const char *errstr;
>   size_t len;
> - int ch, err = 0;
> + int ch, errs = 0;
>  
>   bzero(&vid, sizeof(struct video));
>  
> @@ -1872,21 +1872,21 @@
>   x->cur_adap = strtonum(optarg, 0, 4, &errstr);
>   if (errstr != NULL) {
>   warnx("Xv adaptor '%s' is %s", optarg, errstr);
> - err++;
> + errs++;
>   }
>   break;
>   case 'e':
>   vid.enc = find_enc(optarg);
>   if (vid.enc >= ENC_LAST) {
>   warnx("encoding '%s' is invalid", optarg);
> - err++;
> + errs++;
>   }
>   break;
>   case 'f':
>   len = strlcpy(d->path, optarg, sizeof(d->path));
>   if (len >= sizeof(d->path)) {
>   warnx("file path is too long: %s", optarg);
> - err++;
> + errs++;
>   }
>   break;
>   case 'g':
> @@ -1894,8 +1894,8 @@
>   break;
>   case 'i':
>   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> - warnx("only one input or ouput file allowed");
> - err++;
> + warnx("only one input or output file allowed");
> + errs++;
>   } else {
>   vid.mode = (vid.mode & ~M_IN_DEV) | M_IN_FILE;
>   vid.mmap_on = 0; /* mmap mode does not work for files */
> @@ -1904,15 +1904,15 @@
>   if (len >= sizeof(vid.iofile)) {
>   warnx("input path is too long: %s",
>      optarg);
> - err++;
> + errs++;
>   }
>   }
>   break;
>   case 'o':
>   case 'O':
>   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> - warnx("only one input or ouput file allowed");
> - err++;
> + warnx("only one input or output file allowed");
> + errs++;
>   } else {
>   vid.mode |= M_OUT_FILE;
>   if (ch != 'O')
> @@ -1922,7 +1922,7 @@
>   if (len >= sizeof(vid.iofile)) {
>   warnx("output path is too long: %s",
>      optarg);
> - err++;
> + errs++;
>   }
>   }
>   break;
> @@ -1937,7 +1937,7 @@
>   vid.fps = strtonum(optarg, 1, 100, &errstr);
>   if (errstr != NULL) {
>   warnx("frame rate '%s' is %s", optarg, errstr);
> - err++;
> + errs++;
>   }
>   break;
>   case 's':
> @@ -1947,13 +1947,13 @@
>   vid.verbose++;
>   break;
>   default:
> - err++;
> + errs++;
>   break;
>   }
> - if (err > 0)
> + if (errs > 0)
>   break;
>   }
> - if (err > 0) {
> + if (errs > 0) {
>   usage();
>   cleanup(&vid, 1);
>   }

Reply | Threaded
Open this post in threaded view
|

Re: video(1) pledge (& updated kernel diff)

Sebastien Marie-3
In reply to this post by Landry Breuil-5
On Sun, Dec 30, 2018 at 10:58:58AM +0100, Landry Breuil wrote:

> On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:
> > On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
>
> > I think you pledged too early.
> >
> > "dns unix" are here for XOpenDisplay(), but if the display is remote,
> > "inet" could be needed too. Even if Xv isn't possible on remote display,
> > XOpenDisplay() will be killed by pledge.
> >
> > So ideally, pledge(2) should occurs later.
>
> Yup, after some discussions, moving the pledge calls after setup()
> (which does the file & xv opening) allows to only need stdio in the -i
> case, and 'stdio rpath video' in the others. i thought rpath could be
> dropped but in my testing with video -vvv -O i had a crash upon some
> keys where x tried to open /usr/X11R6/share/X11/locale/locale.alias..

libX11 tentacles :)  I think any program using X11 needs rpath.
 
> in addition we can also pledge the video -q case which needs
> stdio/rpath/wpath/video.

The promises looks more sanely now.

For me, there is still initialization stuff in dev_check_caps(), a
function used to query capabilities of the device (if the device is able
to record video, and how [via read-write or via streaming (mmap)]).

As it is the first function using ioctl(2) on the device, it opens
the descriptor device (using O_RDWR). It is why we need rpath+wpath
here. But such refactoring is outside the scope of the diff. No problem
with that.

Thanks. I will review the kernel side soon.

> Index: video.c
> ===================================================================
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c 9 Apr 2018 18:16:44 -0000 1.25
> +++ video.c 30 Dec 2018 09:39:27 -0000
> @@ -1961,6 +1961,8 @@
>   argv += optind;
>  
>   if (vid.mode & M_QUERY) {
> + if (pledge("stdio rpath wpath video", NULL) == -1)
> + err(1, "pledge");
>   dev_dump_query(&vid);
>   cleanup(&vid, 0);
>   }
> @@ -1970,6 +1972,14 @@
>  
>   if (!setup(&vid))
>   cleanup(&vid, 1);
> +
> + if (vid.mode & M_IN_FILE) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + } else {
> + if (pledge("stdio rpath video", NULL) == -1)
> + err(1, "pledge");
> + }
>  
>   if (!stream(&vid))
>   cleanup(&vid, 1);


--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: video(1) pledge (& updated kernel diff)

Matthieu Herrb-7
In reply to this post by Sebastien Marie-3
On Sun, Dec 30, 2018 at 12:14:58PM +0100, Sebastien Marie wrote:

> On Sun, Dec 30, 2018 at 10:58:58AM +0100, Landry Breuil wrote:
> > On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:
> > > On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> > >
> > > I would separate the addition of pledge(2) and unrelated fixes.
> >
> > Right, two separated diffs attached for this.
> >
>
> the err->errs diff to avoid shadowing is ok semarie@
>
> mherrb@, any objections to it ?

Hi,

sorry for not noticing that I was summoned...
No objection.

>
> Thanks
> --
> Sebastien Marie
>
> > Index: video.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/video/video.c,v
> > retrieving revision 1.25
> > diff -u -r1.25 video.c
> > --- video.c 9 Apr 2018 18:16:44 -0000 1.25
> > +++ video.c 30 Dec 2018 09:39:21 -0000
> > @@ -1854,7 +1854,7 @@
> >   struct xdsp *x = &vid.xdsp;
> >   const char *errstr;
> >   size_t len;
> > - int ch, err = 0;
> > + int ch, errs = 0;
> >  
> >   bzero(&vid, sizeof(struct video));
> >  
> > @@ -1872,21 +1872,21 @@
> >   x->cur_adap = strtonum(optarg, 0, 4, &errstr);
> >   if (errstr != NULL) {
> >   warnx("Xv adaptor '%s' is %s", optarg, errstr);
> > - err++;
> > + errs++;
> >   }
> >   break;
> >   case 'e':
> >   vid.enc = find_enc(optarg);
> >   if (vid.enc >= ENC_LAST) {
> >   warnx("encoding '%s' is invalid", optarg);
> > - err++;
> > + errs++;
> >   }
> >   break;
> >   case 'f':
> >   len = strlcpy(d->path, optarg, sizeof(d->path));
> >   if (len >= sizeof(d->path)) {
> >   warnx("file path is too long: %s", optarg);
> > - err++;
> > + errs++;
> >   }
> >   break;
> >   case 'g':
> > @@ -1894,8 +1894,8 @@
> >   break;
> >   case 'i':
> >   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> > - warnx("only one input or ouput file allowed");
> > - err++;
> > + warnx("only one input or output file allowed");
> > + errs++;
> >   } else {
> >   vid.mode = (vid.mode & ~M_IN_DEV) | M_IN_FILE;
> >   vid.mmap_on = 0; /* mmap mode does not work for files */
> > @@ -1904,15 +1904,15 @@
> >   if (len >= sizeof(vid.iofile)) {
> >   warnx("input path is too long: %s",
> >      optarg);
> > - err++;
> > + errs++;
> >   }
> >   }
> >   break;
> >   case 'o':
> >   case 'O':
> >   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> > - warnx("only one input or ouput file allowed");
> > - err++;
> > + warnx("only one input or output file allowed");
> > + errs++;
> >   } else {
> >   vid.mode |= M_OUT_FILE;
> >   if (ch != 'O')
> > @@ -1922,7 +1922,7 @@
> >   if (len >= sizeof(vid.iofile)) {
> >   warnx("output path is too long: %s",
> >      optarg);
> > - err++;
> > + errs++;
> >   }
> >   }
> >   break;
> > @@ -1937,7 +1937,7 @@
> >   vid.fps = strtonum(optarg, 1, 100, &errstr);
> >   if (errstr != NULL) {
> >   warnx("frame rate '%s' is %s", optarg, errstr);
> > - err++;
> > + errs++;
> >   }
> >   break;
> >   case 's':
> > @@ -1947,13 +1947,13 @@
> >   vid.verbose++;
> >   break;
> >   default:
> > - err++;
> > + errs++;
> >   break;
> >   }
> > - if (err > 0)
> > + if (errs > 0)
> >   break;
> >   }
> > - if (err > 0) {
> > + if (errs > 0) {
> >   usage();
> >   cleanup(&vid, 1);
> >   }

--
Matthieu Herrb