| Message ID | 1338815410-24890-3-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06/04/2012 07:10 AM, Corey Bryant wrote: > The main goal of this patch series is to enable isolation of guest > images that are stored on the same NFS mount. This can be achieved > if the management application opens files for QEMU, and QEMU is > restricted from opening files. > > This patch adds support to the block layer open paths to dup(X) a > pre-opened file descriptor if the filename is of the format > /dev/fd/X. > > One nice thing about this approach is that no new SELinux policy is > required to prevent open of NFS files (files with type nfs_t). The > virt_use_nfs boolean type simply needs to be set to false, and open > will be prevented (yet dup will be allowed). For example: > > # setsebool virt_use_nfs 0 > # getsebool virt_use_nfs > virt_use_nfs --> off > > Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> > > +int file_open(const char *filename, int flags, mode_t mode) > +{ > +#ifndef _WIN32 > + int fd; > + const char *p; > + > + if (strstart(filename, "/dev/fd/", &p)) { > + fd = atoi(p); atoi() is lousy - it has no error checking, and returns 0 if a mistake was made. You really want to be using strtol (or even better, a sensible wrapper around strtol that takes care of the subtleties of calling it correctly), so that you don't end up dup'ing stdin when the user passes a bad /dev/fd/ string.
Am 04.06.2012 15:10, schrieb Corey Bryant: > The main goal of this patch series is to enable isolation of guest > images that are stored on the same NFS mount. This can be achieved > if the management application opens files for QEMU, and QEMU is > restricted from opening files. > > This patch adds support to the block layer open paths to dup(X) a > pre-opened file descriptor if the filename is of the format > /dev/fd/X. > > One nice thing about this approach is that no new SELinux policy is > required to prevent open of NFS files (files with type nfs_t). The > virt_use_nfs boolean type simply needs to be set to false, and open > will be prevented (yet dup will be allowed). For example: > > # setsebool virt_use_nfs 0 > # getsebool virt_use_nfs > virt_use_nfs --> off > > Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> > --- > block.c | 15 +++++++++++++++ > block/raw-posix.c | 20 ++++++++++---------- > block/raw-win32.c | 4 ++-- > block/vdi.c | 5 +++-- > block/vmdk.c | 21 +++++++++------------ > block/vpc.c | 2 +- > block/vvfat.c | 21 +++++++++++---------- > block_int.h | 13 +++++++++++++ > 8 files changed, 64 insertions(+), 37 deletions(-) > > diff --git a/block.c b/block.c > index af2ab4f..8416313 100644 > --- a/block.c > +++ b/block.c > @@ -102,6 +102,21 @@ static BlockDriverState *bs_snapshots; > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > > +int file_open(const char *filename, int flags, mode_t mode) > +{ > +#ifndef _WIN32 > + int fd; > + const char *p; > + > + if (strstart(filename, "/dev/fd/", &p)) { > + fd = atoi(p); > + return dup(fd); > + } > +#endif > + > + return qemu_open(filename, flags, mode); > +} What's the reason for having separate file_open() and qemu_open() functions? Are there places where you don't want allow to use /dev/fd? Otherwise I would have expected that we just extend qemu_open(). I'd have the remaining open -> qemu_open conversions as a separate patch then. They do not only add fd support as advertised in the commit message, but also add O_CLOEXEC. I don't think that's bad, these are likely bonus bug fixes, but they should be mentioned in the commit message. Kevin
On 06/04/2012 10:32 AM, Eric Blake wrote: > On 06/04/2012 07:10 AM, Corey Bryant wrote: >> The main goal of this patch series is to enable isolation of guest >> images that are stored on the same NFS mount. This can be achieved >> if the management application opens files for QEMU, and QEMU is >> restricted from opening files. >> >> This patch adds support to the block layer open paths to dup(X) a >> pre-opened file descriptor if the filename is of the format >> /dev/fd/X. >> >> One nice thing about this approach is that no new SELinux policy is >> required to prevent open of NFS files (files with type nfs_t). The >> virt_use_nfs boolean type simply needs to be set to false, and open >> will be prevented (yet dup will be allowed). For example: >> >> # setsebool virt_use_nfs 0 >> # getsebool virt_use_nfs >> virt_use_nfs --> off >> >> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> > >> >> +int file_open(const char *filename, int flags, mode_t mode) >> +{ >> +#ifndef _WIN32 >> + int fd; >> + const char *p; >> + >> + if (strstart(filename, "/dev/fd/",&p)) { >> + fd = atoi(p); > > atoi() is lousy - it has no error checking, and returns 0 if a mistake > was made. You really want to be using strtol (or even better, a > sensible wrapper around strtol that takes care of the subtleties of > calling it correctly), so that you don't end up dup'ing stdin when the > user passes a bad /dev/fd/ string. > It looks like strtol returns 0 on failure too. Do we need to support stdin/stdout/stderr?
On 06/04/2012 09:51 AM, Corey Bryant wrote: >>> + >>> + if (strstart(filename, "/dev/fd/",&p)) { >>> + fd = atoi(p); >> >> atoi() is lousy - it has no error checking, and returns 0 if a mistake >> was made. You really want to be using strtol (or even better, a >> sensible wrapper around strtol that takes care of the subtleties of >> calling it correctly), so that you don't end up dup'ing stdin when the >> user passes a bad /dev/fd/ string. >> > > It looks like strtol returns 0 on failure too. Do we need to support > stdin/stdout/stderr? But at least strtol lets you detect errors: char *tmp; errno = 0; fd = strtol(p, &tmp, 10); if (errno || tmp == p) { /* raise your error here */ } and if you get past that point, then someone really did pass in /dev/fd/0 as the string they meant to be parsed (probably a user bug, as getfd is unlikely to ever return 0 unless you start with stdin closed, which itself is something that POSIX discourages, but not something we need to specifically worry about). So I would argue that yes, we do need to support fd 0, if only by not special casing it as compared to any other valid fd.
On 06/04/2012 10:54 AM, Kevin Wolf wrote: > Am 04.06.2012 15:10, schrieb Corey Bryant: >> The main goal of this patch series is to enable isolation of guest >> images that are stored on the same NFS mount. This can be achieved >> if the management application opens files for QEMU, and QEMU is >> restricted from opening files. >> >> This patch adds support to the block layer open paths to dup(X) a >> pre-opened file descriptor if the filename is of the format >> /dev/fd/X. >> >> One nice thing about this approach is that no new SELinux policy is >> required to prevent open of NFS files (files with type nfs_t). The >> virt_use_nfs boolean type simply needs to be set to false, and open >> will be prevented (yet dup will be allowed). For example: >> >> # setsebool virt_use_nfs 0 >> # getsebool virt_use_nfs >> virt_use_nfs --> off >> >> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> >> --- >> block.c | 15 +++++++++++++++ >> block/raw-posix.c | 20 ++++++++++---------- >> block/raw-win32.c | 4 ++-- >> block/vdi.c | 5 +++-- >> block/vmdk.c | 21 +++++++++------------ >> block/vpc.c | 2 +- >> block/vvfat.c | 21 +++++++++++---------- >> block_int.h | 13 +++++++++++++ >> 8 files changed, 64 insertions(+), 37 deletions(-) >> >> diff --git a/block.c b/block.c >> index af2ab4f..8416313 100644 >> --- a/block.c >> +++ b/block.c >> @@ -102,6 +102,21 @@ static BlockDriverState *bs_snapshots; >> /* If non-zero, use only whitelisted block drivers */ >> static int use_bdrv_whitelist; >> >> +int file_open(const char *filename, int flags, mode_t mode) >> +{ >> +#ifndef _WIN32 >> + int fd; >> + const char *p; >> + >> + if (strstart(filename, "/dev/fd/",&p)) { >> + fd = atoi(p); >> + return dup(fd); >> + } >> +#endif >> + >> + return qemu_open(filename, flags, mode); >> +} > > What's the reason for having separate file_open() and qemu_open() > functions? Are there places where you don't want allow to use /dev/fd? > Otherwise I would have expected that we just extend qemu_open(). I'm not sure there's any good reason to have a separate file_open() vs just adding this code to qemu_open(). I followed the lead of Anthony's prototype which used file_open() so he may have a reason. Otherwise I'll move the code to qemu_open() in v2. > > I'd have the remaining open -> qemu_open conversions as a separate patch > then. They do not only add fd support as advertised in the commit > message, but also add O_CLOEXEC. I don't think that's bad, these are > likely bonus bug fixes, but they should be mentioned in the commit message. Yes good point. I'll be sure to note the addition of O_CLOEXEC for these cases in the commit message.
On 06/04/2012 12:03 PM, Eric Blake wrote: > On 06/04/2012 09:51 AM, Corey Bryant wrote: > >>>> + >>>> + if (strstart(filename, "/dev/fd/",&p)) { >>>> + fd = atoi(p); >>> >>> atoi() is lousy - it has no error checking, and returns 0 if a mistake >>> was made. You really want to be using strtol (or even better, a >>> sensible wrapper around strtol that takes care of the subtleties of >>> calling it correctly), so that you don't end up dup'ing stdin when the >>> user passes a bad /dev/fd/ string. >>> >> >> It looks like strtol returns 0 on failure too. Do we need to support >> stdin/stdout/stderr? > > But at least strtol lets you detect errors: > > char *tmp; > errno = 0; > fd = strtol(p,&tmp, 10); > if (errno || tmp == p) { > /* raise your error here */ > } I don't think this is legitimate. errno can be set under the covers of library calls even if the strtol() call is successful. I was thinking if strtol returns 0 and errno is 0, perhaps we could assume success, but I don't think this is guaranteed either. Maybe a combination of isdigit() then strtol() will give a better idea of success. > > and if you get past that point, then someone really did pass in > /dev/fd/0 as the string they meant to be parsed (probably a user bug, as > getfd is unlikely to ever return 0 unless you start with stdin closed, > which itself is something that POSIX discourages, but not something we > need to specifically worry about). So I would argue that yes, we do > need to support fd 0, if only by not special casing it as compared to > any other valid fd. > Ok
On 06/04/2012 10:28 AM, Corey Bryant wrote: >> But at least strtol lets you detect errors: >> >> char *tmp; >> errno = 0; >> fd = strtol(p,&tmp, 10); >> if (errno || tmp == p) { >> /* raise your error here */ >> } > > I don't think this is legitimate. errno can be set under the covers of > library calls even if the strtol() call is successful. Wrong. POSIX _specifically_ requires that strtol() leave errno unchanged unless strtol() is reporting a failure, no matter what other library calls (if any) are made under the covers by strtol(). In other words, pre-setting errno to 0, then calling strtol(), then checking errno, _is_ the documented way to check for strtol() failures, and a correct usage of strtol() MUST use this method. See also commit 6b0e33be88bbccc3bcb987026089aa09f9622de9. atoi() does not have this same guarantee, which makes atoi() worthless at detecting errors in relation to strtol(). > > I was thinking if strtol returns 0 and errno is 0, perhaps we could > assume success, but I don't think this is guaranteed either. Actually, it _is_ guaranteed - if you pre-set errno to 0, then call strtol(), then errno is still 0, then the result did not encounter an error, so a result of 0 at that point means that you indeed parsed a 0. > > Maybe a combination of isdigit() then strtol() will give a better idea > of success. Not necessary.
On 06/04/2012 12:36 PM, Eric Blake wrote: > On 06/04/2012 10:28 AM, Corey Bryant wrote: > >>> But at least strtol lets you detect errors: >>> >>> char *tmp; >>> errno = 0; >>> fd = strtol(p,&tmp, 10); >>> if (errno || tmp == p) { >>> /* raise your error here */ >>> } >> >> I don't think this is legitimate. errno can be set under the covers of >> library calls even if the strtol() call is successful. > > Wrong. POSIX _specifically_ requires that strtol() leave errno > unchanged unless strtol() is reporting a failure, no matter what other > library calls (if any) are made under the covers by strtol(). > > In other words, pre-setting errno to 0, then calling strtol(), then > checking errno, _is_ the documented way to check for strtol() failures, > and a correct usage of strtol() MUST use this method. See also commit > 6b0e33be88bbccc3bcb987026089aa09f9622de9. atoi() does not have this > same guarantee, which makes atoi() worthless at detecting errors in > relation to strtol(). > Great! I see it now (excerpt below is from the opengroup specification). This is definitely an exception from normal behavior of C APIs. "The strtol() function will not change the setting of errno if successful." >> >> I was thinking if strtol returns 0 and errno is 0, perhaps we could >> assume success, but I don't think this is guaranteed either. > > Actually, it _is_ guaranteed - if you pre-set errno to 0, then call > strtol(), then errno is still 0, then the result did not encounter an > error, so a result of 0 at that point means that you indeed parsed a 0. > >> >> Maybe a combination of isdigit() then strtol() will give a better idea >> of success. > > Not necessary. >
diff --git a/block.c b/block.c index af2ab4f..8416313 100644 --- a/block.c +++ b/block.c @@ -102,6 +102,21 @@ static BlockDriverState *bs_snapshots; /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; +int file_open(const char *filename, int flags, mode_t mode) +{ +#ifndef _WIN32 + int fd; + const char *p; + + if (strstart(filename, "/dev/fd/", &p)) { + fd = atoi(p); + return dup(fd); + } +#endif + + return qemu_open(filename, flags, mode); +} + #ifdef _WIN32 static int is_windows_drive_prefix(const char *filename) { diff --git a/block/raw-posix.c b/block/raw-posix.c index 03fcfcc..4f7b40f 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -208,7 +208,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, s->open_flags |= O_DSYNC; s->fd = -1; - fd = qemu_open(filename, s->open_flags, 0644); + fd = file_open(filename, s->open_flags, 0644); if (fd < 0) { ret = -errno; if (ret == -EROFS) @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = file_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) { result = -errno; } else { @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = file_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE, 0); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = file_open(bs->filename, s->open_flags & ~O_NONBLOCK, 0); if (s->fd < 0) { s->fd_error_time = get_clock(); s->fd_got_error = 1; @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_BINARY); + fd = file_open(filename, O_WRONLY | O_BINARY, 0); if (fd < 0) return -errno; @@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename) if (strstart(filename, "/dev/fd", NULL)) prio = 50; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = file_open(filename, O_RDONLY | O_NONBLOCK, 0); if (fd < 0) { goto out; } @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = file_open(bs->filename, s->open_flags | O_NONBLOCK, 0); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename) int prio = 0; struct stat st; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = file_open(filename, O_RDONLY | O_NONBLOCK, 0); if (fd < 0) { goto out; } @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = file_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index e4b0b75..ec4ac96 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = file_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) return -EIO; set_sparse(fd); diff --git a/block/vdi.c b/block/vdi.c index 119d3c7..bd0f4b5 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = file_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } diff --git a/block/vmdk.c b/block/vmdk.c index 18e9b4c..bda4c1a 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, VMDK4Header header; uint32_t tmp, magic, grains, gd_size, gt_size, gt_count; - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = file_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), total_size / (int64_t)(63 * 16 * 512)); if (split || flat) { - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = file_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); } else { - fd = open( - filename, - O_WRONLY | O_BINARY | O_LARGEFILE, - 0644); + fd = file_open(filename, + O_WRONLY | O_BINARY | O_LARGEFILE, + 0644); } if (fd < 0) { return -errno; diff --git a/block/vpc.c b/block/vpc.c index 5cd13d1..c1efb10 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) } /* Create the file */ - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); + fd = file_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { return -EIO; } diff --git a/block/vvfat.c b/block/vvfat.c index 2dc9d50..555e295 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_ static int open_file(BDRVVVFATState* s,mapping_t* mapping) { if(!mapping) - return -1; + return -1; if(!s->current_mapping || - strcmp(s->current_mapping->path,mapping->path)) { - /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); - if(fd<0) - return -1; - vvfat_close_current_file(s); - s->current_fd = fd; - s->current_mapping = mapping; + strcmp(s->current_mapping->path, mapping->path)) { + /* open file */ + int fd = file_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE, 0); + if (fd < 0) { + return -1; + } + vvfat_close_current_file(s); + s->current_fd = fd; + s->current_mapping = mapping; } return 0; } @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s, for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c); - fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); + fd = file_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); if (fd < 0) { fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path, strerror(errno), errno); diff --git a/block_int.h b/block_int.h index b80e66d..2510713 100644 --- a/block_int.h +++ b/block_int.h @@ -453,4 +453,17 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverCompletionFunc *cb, void *opaque, Error **errp); +/** + * file_open: + * @filename: the filename to attempt to open + * @flags: O_ flags that determine how the file is opened + * @mode: the mode to create the file with if #O_CREAT is included in @flags + * + * This function behaves just like the @open libc function. However, it can + * also duplicate the file descriptor of a pre-opened file if the filename + * is of the format /dev/fd/X. This is useful when QEMU is restricted from + * opening a specific type of file. + */ +int file_open(const char *filename, int flags, mode_t mode); + #endif /* BLOCK_INT_H */
The main goal of this patch series is to enable isolation of guest images that are stored on the same NFS mount. This can be achieved if the management application opens files for QEMU, and QEMU is restricted from opening files. This patch adds support to the block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. One nice thing about this approach is that no new SELinux policy is required to prevent open of NFS files (files with type nfs_t). The virt_use_nfs boolean type simply needs to be set to false, and open will be prevented (yet dup will be allowed). For example: # setsebool virt_use_nfs 0 # getsebool virt_use_nfs virt_use_nfs --> off Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- block.c | 15 +++++++++++++++ block/raw-posix.c | 20 ++++++++++---------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 +++++++++------------ block/vpc.c | 2 +- block/vvfat.c | 21 +++++++++++---------- block_int.h | 13 +++++++++++++ 8 files changed, 64 insertions(+), 37 deletions(-)