Message ID | 20191212163904.159893-63-dgilbert@redhat.com |
---|---|
State | New |
Headers | show |
Series | virtiofs daemon [all] | expand |
On Thu, Dec 12, 2019 at 04:38:22PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Handle a > mount > hard reboot (without unmount) > mount > > we get another 'init' which FUSE doesn't normally expect. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tools/virtiofsd/fuse_lowlevel.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c > index 2d1d1a2e59..45125ef66a 100644 > --- a/tools/virtiofsd/fuse_lowlevel.c > +++ b/tools/virtiofsd/fuse_lowlevel.c > @@ -2436,7 +2436,21 @@ void fuse_session_process_buf_int(struct fuse_session *se, > goto reply_err; > } > } else if (in->opcode == FUSE_INIT || in->opcode == CUSE_INIT) { > - goto reply_err; > + if (fuse_lowlevel_is_virtio(se)) { > + /* > + * TODO: This is after a hard reboot typically, we need to do > + * a destroy, but we can't reply to this request yet so > + * we can't use do_destroy > + */ > + fuse_log(FUSE_LOG_DEBUG, "%s: reinit\n", __func__); > + se->got_destroy = 1; > + se->got_init = 0; > + if (se->op.destroy) { > + se->op.destroy(se->userdata); > + } > + } else { > + goto reply_err; > + } In doing this, is there any danger we're exposed to from a malicious guest which does mount mount without a reboot in between ? I'm thinking not so if its ok, then Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Thu, Dec 12, 2019 at 04:38:22PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Handle a > > mount > > hard reboot (without unmount) > > mount > > > > we get another 'init' which FUSE doesn't normally expect. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tools/virtiofsd/fuse_lowlevel.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c > > index 2d1d1a2e59..45125ef66a 100644 > > --- a/tools/virtiofsd/fuse_lowlevel.c > > +++ b/tools/virtiofsd/fuse_lowlevel.c > > @@ -2436,7 +2436,21 @@ void fuse_session_process_buf_int(struct fuse_session *se, > > goto reply_err; > > } > > } else if (in->opcode == FUSE_INIT || in->opcode == CUSE_INIT) { > > - goto reply_err; > > + if (fuse_lowlevel_is_virtio(se)) { > > + /* > > + * TODO: This is after a hard reboot typically, we need to do > > + * a destroy, but we can't reply to this request yet so > > + * we can't use do_destroy > > + */ > > + fuse_log(FUSE_LOG_DEBUG, "%s: reinit\n", __func__); > > + se->got_destroy = 1; > > + se->got_init = 0; > > + if (se->op.destroy) { > > + se->op.destroy(se->userdata); > > + } > > + } else { > > + goto reply_err; > > + } > > In doing this, is there any danger we're exposed to from a malicious > guest which does > > mount > mount > > without a reboot in between ? I don't think so - or at least not from the daemon side of things; if it were to do that (and get two FUSE_INIT's) then the state of it's first mount would be rather messed up; but the only thing to suffer would be the kernel doing that odd re-init, so I don't think the maliciousness should break anyone else. > I'm thinking not so if its ok, then > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thanks. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Handle a > mount > hard reboot (without unmount) > mount > > we get another 'init' which FUSE doesn't normally expect. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tools/virtiofsd/fuse_lowlevel.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c > index 2d1d1a2e59..45125ef66a 100644 > --- a/tools/virtiofsd/fuse_lowlevel.c > +++ b/tools/virtiofsd/fuse_lowlevel.c > @@ -2436,7 +2436,21 @@ void fuse_session_process_buf_int(struct fuse_session *se, > goto reply_err; > } > } else if (in->opcode == FUSE_INIT || in->opcode == CUSE_INIT) { > - goto reply_err; > + if (fuse_lowlevel_is_virtio(se)) { > + /* > + * TODO: This is after a hard reboot typically, we need to do > + * a destroy, but we can't reply to this request yet so > + * we can't use do_destroy > + */ Hi, I wonder what is the TODO actually. Is this just to provide a common function for both here and do_destroy() or more than that? Thanks Misono > + fuse_log(FUSE_LOG_DEBUG, "%s: reinit\n", __func__); > + se->got_destroy = 1; > + se->got_init = 0; > + if (se->op.destroy) { > + se->op.destroy(se->userdata); > + } > + } else { > + goto reply_err; > + } > } > > err = EACCES; > -- > 2.23.0
* Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Handle a > > mount > > hard reboot (without unmount) > > mount > > > > we get another 'init' which FUSE doesn't normally expect. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tools/virtiofsd/fuse_lowlevel.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c > > index 2d1d1a2e59..45125ef66a 100644 > > --- a/tools/virtiofsd/fuse_lowlevel.c > > +++ b/tools/virtiofsd/fuse_lowlevel.c > > @@ -2436,7 +2436,21 @@ void fuse_session_process_buf_int(struct fuse_session *se, > > goto reply_err; > > } > > } else if (in->opcode == FUSE_INIT || in->opcode == CUSE_INIT) { > > - goto reply_err; > > + if (fuse_lowlevel_is_virtio(se)) { > > > + /* > > + * TODO: This is after a hard reboot typically, we need to do > > + * a destroy, but we can't reply to this request yet so > > + * we can't use do_destroy > > + */ > > Hi, > > I wonder what is the TODO actually. Is this just to provide a common > function for both here and do_destroy() or more than that? Yes, we really need to combine it somehow; but do_destroy is based n responding to a request, but we don't have a normal request at this point. Dave > Thanks > Misono > > > + fuse_log(FUSE_LOG_DEBUG, "%s: reinit\n", __func__); > > + se->got_destroy = 1; > > + se->got_init = 0; > > + if (se->op.destroy) { > > + se->op.destroy(se->userdata); > > + } > > + } else { > > + goto reply_err; > > + } > > } > > > > err = EACCES; > > -- > > 2.23.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c index 2d1d1a2e59..45125ef66a 100644 --- a/tools/virtiofsd/fuse_lowlevel.c +++ b/tools/virtiofsd/fuse_lowlevel.c @@ -2436,7 +2436,21 @@ void fuse_session_process_buf_int(struct fuse_session *se, goto reply_err; } } else if (in->opcode == FUSE_INIT || in->opcode == CUSE_INIT) { - goto reply_err; + if (fuse_lowlevel_is_virtio(se)) { + /* + * TODO: This is after a hard reboot typically, we need to do + * a destroy, but we can't reply to this request yet so + * we can't use do_destroy + */ + fuse_log(FUSE_LOG_DEBUG, "%s: reinit\n", __func__); + se->got_destroy = 1; + se->got_init = 0; + if (se->op.destroy) { + se->op.destroy(se->userdata); + } + } else { + goto reply_err; + } } err = EACCES;