Re: VMD: revise check for regular files on disks

[Carlos Cardenas]

Jeremie Courreges-Anglas  wrote:

> On Wed, Jan 03 2018, Carlos Cardenas  wrote:
> > Howdy.
> >
> > Attached is a patch to address a TOCTOU issue with checking to
> > ensure disks are regular files, reported by jca@ .
> >
> > Comments? Ok?
> 
> A bit late, but ok.
> 
> While here, if the S_ISREG check fails there is no meaningful errno to
> report.
> 
> ok?
> 
ok ccardenas
> 
> Index: config.c
> ===
> RCS file: /d/cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.39
> diff -u -p -p -u -r1.39 config.c
> --- config.c  4 Jan 2018 15:19:56 -   1.39
> +++ config.c  5 Jan 2018 07:24:41 -
> @@ -252,7 +252,7 @@ config_setvm(struct privsep *ps, struct 
>   goto fail;
>   }
>   if (S_ISREG(stat_buf.st_mode) == 0) {
> - log_warn("%s: cdrom %s is not a regular file", __func__,
> + log_warnx("%s: cdrom %s is not a regular file", 
> __func__,
>   vcp->vcp_cdrom);
>   errno = VMD_CDROM_INVALID;
>   goto fail;
> @@ -276,7 +276,7 @@ config_setvm(struct privsep *ps, struct 
>   goto fail;
>   }
>   if (S_ISREG(stat_buf.st_mode) == 0) {
> - log_warn("%s: disk %s is not a regular file", __func__,
> + log_warnx("%s: disk %s is not a regular file", __func__,
>   vcp->vcp_disks[i]);
>   errno = VMD_DISK_INVALID;
>   goto fail;
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: VMD: revise check for regular files on disks

[Mike Larkin]

On Fri, Jan 05, 2018 at 08:26:21AM +0100, Jeremie Courreges-Anglas wrote:
> On Wed, Jan 03 2018, Carlos Cardenas  wrote:
> > Howdy.
> >
> > Attached is a patch to address a TOCTOU issue with checking to
> > ensure disks are regular files, reported by jca@ .
> >
> > Comments? Ok?
> 
> A bit late, but ok.
> 
> While here, if the S_ISREG check fails there is no meaningful errno to
> report.
> 
> ok?
> 

ok mlarkin

> 
> Index: config.c
> ===
> RCS file: /d/cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.39
> diff -u -p -p -u -r1.39 config.c
> --- config.c  4 Jan 2018 15:19:56 -   1.39
> +++ config.c  5 Jan 2018 07:24:41 -
> @@ -252,7 +252,7 @@ config_setvm(struct privsep *ps, struct 
>   goto fail;
>   }
>   if (S_ISREG(stat_buf.st_mode) == 0) {
> - log_warn("%s: cdrom %s is not a regular file", __func__,
> + log_warnx("%s: cdrom %s is not a regular file", 
> __func__,
>   vcp->vcp_cdrom);
>   errno = VMD_CDROM_INVALID;
>   goto fail;
> @@ -276,7 +276,7 @@ config_setvm(struct privsep *ps, struct 
>   goto fail;
>   }
>   if (S_ISREG(stat_buf.st_mode) == 0) {
> - log_warn("%s: disk %s is not a regular file", __func__,
> + log_warnx("%s: disk %s is not a regular file", __func__,
>   vcp->vcp_disks[i]);
>   errno = VMD_DISK_INVALID;
>   goto fail;
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: VMD: revise check for regular files on disks

[Jeremie Courreges-Anglas]

On Wed, Jan 03 2018, Carlos Cardenas  wrote:
> Howdy.
>
> Attached is a patch to address a TOCTOU issue with checking to
> ensure disks are regular files, reported by jca@ .
>
> Comments? Ok?

A bit late, but ok.

While here, if the S_ISREG check fails there is no meaningful errno to
report.

ok?


Index: config.c
===
RCS file: /d/cvs/src/usr.sbin/vmd/config.c,v
retrieving revision 1.39
diff -u -p -p -u -r1.39 config.c
--- config.c4 Jan 2018 15:19:56 -   1.39
+++ config.c5 Jan 2018 07:24:41 -
@@ -252,7 +252,7 @@ config_setvm(struct privsep *ps, struct 
goto fail;
}
if (S_ISREG(stat_buf.st_mode) == 0) {
-   log_warn("%s: cdrom %s is not a regular file", __func__,
+   log_warnx("%s: cdrom %s is not a regular file", 
__func__,
vcp->vcp_cdrom);
errno = VMD_CDROM_INVALID;
goto fail;
@@ -276,7 +276,7 @@ config_setvm(struct privsep *ps, struct 
goto fail;
}
if (S_ISREG(stat_buf.st_mode) == 0) {
-   log_warn("%s: disk %s is not a regular file", __func__,
+   log_warnx("%s: disk %s is not a regular file", __func__,
vcp->vcp_disks[i]);
errno = VMD_DISK_INVALID;
goto fail;

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: VMD: revise check for regular files on disks

[Mike Larkin]

On Thu, Jan 04, 2018 at 07:14:54AM -0800, Carlos Cardenas wrote:
> Mike Larkin  wrote:
> 
> > On Wed, Jan 03, 2018 at 08:03:56PM -0800, Carlos Cardenas wrote:
> > > Howdy.
> > > 
> > > Attached is a patch to address a TOCTOU issue with checking to
> > > ensure disks are regular files, reported by jca@ .
> > > 
> > > Comments? Ok?
> > > 
> > > +--+
> > > Carlos
> > 
> > > Index: config.c
> > > ===
> > > RCS file: /home/los/cvs/src/usr.sbin/vmd/config.c,v
> > > retrieving revision 1.38
> > > diff -u -p -a -u -r1.38 config.c
> > > --- config.c  3 Jan 2018 05:39:56 -   1.38
> > > +++ config.c  4 Jan 2018 03:55:47 -
> > > @@ -262,23 +262,23 @@ config_setvm(struct privsep *ps, struct 
> > >   /* Open disk images for child */
> > >   for (i = 0 ; i < vcp->vcp_ndisks; i++) {
> > >  /* Stat disk[i] to ensure it is a regular file */
> > > - if (stat(vcp->vcp_disks[i], _buf) == -1) {
> > > + if ((diskfds[i] =
> > > + open(vcp->vcp_disks[i], O_RDWR)) == -1) {
> > 
> > O_RDONLY? Or do we actually support the SCSI write commands (ala
> > writing ISO images?)
> 
> vcp_disks represent the vioblk devices which are RDWR.
> vcp_cdrom is RDONLY since it doesn't support writing ISOs.
> 

Of course. I missed that bit. You're right. I thought this was only for
the recent cdrom changes. No concern then.

> > 
> > >   log_warn("%s: can't open disk %s", __func__,
> > >   vcp->vcp_disks[i]);
> > >   errno = VMD_DISK_MISSING;
> > >   goto fail;
> > >   }
> > > - if (S_ISREG(stat_buf.st_mode) == 0) {
> > > - log_warn("%s: disk %s is not a regular file", __func__,
> > > + if (fstat(diskfds[i], _buf) == -1) {
> > > + log_warn("%s: can't open disk %s", __func__,
> > >   vcp->vcp_disks[i]);
> > > - errno = VMD_DISK_INVALID;
> > > + errno = VMD_DISK_MISSING;
> > 
> > I'd probably stick with INVALID here since technically the image is not
> > really "missing"
> 
> Makes sense.
> 
> > 
> > >   goto fail;
> > >   }
> > > - if ((diskfds[i] =
> > > - open(vcp->vcp_disks[i], O_RDWR)) == -1) {
> > > - log_warn("%s: can't open disk %s", __func__,
> > > + if (S_ISREG(stat_buf.st_mode) == 0) {
> > > + log_warn("%s: disk %s is not a regular file", __func__,
> > >   vcp->vcp_disks[i]);
> > > - errno = VMD_DISK_MISSING;
> > > + errno = VMD_DISK_INVALID;
> > >   goto fail;
> > >   }
> > >   }
> > 
> > ok mlarkin otherwise

Re: VMD: revise check for regular files on disks

[Carlos Cardenas]

Mike Larkin  wrote:

> On Wed, Jan 03, 2018 at 08:03:56PM -0800, Carlos Cardenas wrote:
> > Howdy.
> > 
> > Attached is a patch to address a TOCTOU issue with checking to
> > ensure disks are regular files, reported by jca@ .
> > 
> > Comments? Ok?
> > 
> > +--+
> > Carlos
> 
> > Index: config.c
> > ===
> > RCS file: /home/los/cvs/src/usr.sbin/vmd/config.c,v
> > retrieving revision 1.38
> > diff -u -p -a -u -r1.38 config.c
> > --- config.c3 Jan 2018 05:39:56 -   1.38
> > +++ config.c4 Jan 2018 03:55:47 -
> > @@ -262,23 +262,23 @@ config_setvm(struct privsep *ps, struct 
> > /* Open disk images for child */
> > for (i = 0 ; i < vcp->vcp_ndisks; i++) {
> >  /* Stat disk[i] to ensure it is a regular file */
> > -   if (stat(vcp->vcp_disks[i], _buf) == -1) {
> > +   if ((diskfds[i] =
> > +   open(vcp->vcp_disks[i], O_RDWR)) == -1) {
> 
> O_RDONLY? Or do we actually support the SCSI write commands (ala
> writing ISO images?)

vcp_disks represent the vioblk devices which are RDWR.
vcp_cdrom is RDONLY since it doesn't support writing ISOs.

> 
> > log_warn("%s: can't open disk %s", __func__,
> > vcp->vcp_disks[i]);
> > errno = VMD_DISK_MISSING;
> > goto fail;
> > }
> > -   if (S_ISREG(stat_buf.st_mode) == 0) {
> > -   log_warn("%s: disk %s is not a regular file", __func__,
> > +   if (fstat(diskfds[i], _buf) == -1) {
> > +   log_warn("%s: can't open disk %s", __func__,
> > vcp->vcp_disks[i]);
> > -   errno = VMD_DISK_INVALID;
> > +   errno = VMD_DISK_MISSING;
> 
> I'd probably stick with INVALID here since technically the image is not
> really "missing"

Makes sense.

> 
> > goto fail;
> > }
> > -   if ((diskfds[i] =
> > -   open(vcp->vcp_disks[i], O_RDWR)) == -1) {
> > -   log_warn("%s: can't open disk %s", __func__,
> > +   if (S_ISREG(stat_buf.st_mode) == 0) {
> > +   log_warn("%s: disk %s is not a regular file", __func__,
> > vcp->vcp_disks[i]);
> > -   errno = VMD_DISK_MISSING;
> > +   errno = VMD_DISK_INVALID;
> > goto fail;
> > }
> > }
> 
> ok mlarkin otherwise

Re: VMD: revise check for regular files on disks

[Mike Larkin]

On Wed, Jan 03, 2018 at 08:03:56PM -0800, Carlos Cardenas wrote:
> Howdy.
> 
> Attached is a patch to address a TOCTOU issue with checking to
> ensure disks are regular files, reported by jca@ .
> 
> Comments? Ok?
> 
> +--+
> Carlos

> Index: config.c
> ===
> RCS file: /home/los/cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.38
> diff -u -p -a -u -r1.38 config.c
> --- config.c  3 Jan 2018 05:39:56 -   1.38
> +++ config.c  4 Jan 2018 03:55:47 -
> @@ -262,23 +262,23 @@ config_setvm(struct privsep *ps, struct 
>   /* Open disk images for child */
>   for (i = 0 ; i < vcp->vcp_ndisks; i++) {
>  /* Stat disk[i] to ensure it is a regular file */
> - if (stat(vcp->vcp_disks[i], _buf) == -1) {
> + if ((diskfds[i] =
> + open(vcp->vcp_disks[i], O_RDWR)) == -1) {

O_RDONLY? Or do we actually support the SCSI write commands (ala
writing ISO images?)

>   log_warn("%s: can't open disk %s", __func__,
>   vcp->vcp_disks[i]);
>   errno = VMD_DISK_MISSING;
>   goto fail;
>   }
> - if (S_ISREG(stat_buf.st_mode) == 0) {
> - log_warn("%s: disk %s is not a regular file", __func__,
> + if (fstat(diskfds[i], _buf) == -1) {
> + log_warn("%s: can't open disk %s", __func__,
>   vcp->vcp_disks[i]);
> - errno = VMD_DISK_INVALID;
> + errno = VMD_DISK_MISSING;

I'd probably stick with INVALID here since technically the image is not
really "missing"

>   goto fail;
>   }
> - if ((diskfds[i] =
> - open(vcp->vcp_disks[i], O_RDWR)) == -1) {
> - log_warn("%s: can't open disk %s", __func__,
> + if (S_ISREG(stat_buf.st_mode) == 0) {
> + log_warn("%s: disk %s is not a regular file", __func__,
>   vcp->vcp_disks[i]);
> - errno = VMD_DISK_MISSING;
> + errno = VMD_DISK_INVALID;
>   goto fail;
>   }
>   }

ok mlarkin otherwise

VMD: revise check for regular files on disks

[Carlos Cardenas]

Howdy.

Attached is a patch to address a TOCTOU issue with checking to
ensure disks are regular files, reported by jca@ .

Comments? Ok?

+--+
Carlos
Index: config.c
===
RCS file: /home/los/cvs/src/usr.sbin/vmd/config.c,v
retrieving revision 1.38
diff -u -p -a -u -r1.38 config.c
--- config.c3 Jan 2018 05:39:56 -   1.38
+++ config.c4 Jan 2018 03:55:47 -
@@ -262,23 +262,23 @@ config_setvm(struct privsep *ps, struct 
/* Open disk images for child */
for (i = 0 ; i < vcp->vcp_ndisks; i++) {
 /* Stat disk[i] to ensure it is a regular file */
-   if (stat(vcp->vcp_disks[i], _buf) == -1) {
+   if ((diskfds[i] =
+   open(vcp->vcp_disks[i], O_RDWR)) == -1) {
log_warn("%s: can't open disk %s", __func__,
vcp->vcp_disks[i]);
errno = VMD_DISK_MISSING;
goto fail;
}
-   if (S_ISREG(stat_buf.st_mode) == 0) {
-   log_warn("%s: disk %s is not a regular file", __func__,
+   if (fstat(diskfds[i], _buf) == -1) {
+   log_warn("%s: can't open disk %s", __func__,
vcp->vcp_disks[i]);
-   errno = VMD_DISK_INVALID;
+   errno = VMD_DISK_MISSING;
goto fail;
}
-   if ((diskfds[i] =
-   open(vcp->vcp_disks[i], O_RDWR)) == -1) {
-   log_warn("%s: can't open disk %s", __func__,
+   if (S_ISREG(stat_buf.st_mode) == 0) {
+   log_warn("%s: disk %s is not a regular file", __func__,
vcp->vcp_disks[i]);
-   errno = VMD_DISK_MISSING;
+   errno = VMD_DISK_INVALID;
goto fail;
}
}