On Mon, Nov 25, 2019 at 03:08:39PM +0100, Hans de Goede wrote:
> + list_for_each_entry(b, &sf_d->info_list, head) {
> +try_next_entry:
> + if (ctx->pos >= cur + b->entries) {
> + cur += b->entries;
> + continue;
> + }
> +
> + /*
> + * Note the vboxsf_dir_info objects we are iterating over here
> + * are variable sized, so the info pointer may end up being
> + * unaligned. This is how we get the data from the host.
> + * Since vboxsf is only supported on x86 machines this is not
> + * a problem.
> + */
> + for (i = 0, info = b->buf; i < ctx->pos - cur; i++) {
> + size = offsetof(struct shfl_dirinfo, name.string) +
> + info->name.size;
> + info = (struct shfl_dirinfo *)((uintptr_t)info + size);
Yecchhh...
1) end = &info->name.string[info->name.size];
info = (struct shfl_dirinfo *)end;
please. Compiler can and will optimize it just fine.
2) what guarantees the lack of overruns here?
> +{
> + bool keep_iterating;
> +
> + for (keep_iterating = true; keep_iterating; ctx->pos += 1)
> + keep_iterating = vboxsf_dir_emit(dir, ctx);
Are you sure you want to bump ctx->pos when vboxsf_dir_emit() returns false?
> +static int vboxsf_dir_create(struct inode *parent, struct dentry *dentry,
> + umode_t mode, int is_dir)
> +{
> + struct vboxsf_inode *sf_parent_i = VBOXSF_I(parent);
> + struct vboxsf_sbi *sbi = VBOXSF_SBI(parent->i_sb);
> + struct shfl_createparms params = {};
> + int err;
> +
> + params.handle = SHFL_HANDLE_NIL;
> + params.create_flags = SHFL_CF_ACT_CREATE_IF_NEW |
> + SHFL_CF_ACT_FAIL_IF_EXISTS |
> + SHFL_CF_ACCESS_READWRITE |
> + (is_dir ? SHFL_CF_DIRECTORY : 0);
> + params.info.attr.mode = (mode & 0777) |
> + (is_dir ? SHFL_TYPE_DIRECTORY : SHFL_TYPE_FILE);
> + params.info.attr.additional = SHFLFSOBJATTRADD_NOTHING;
> +
> + err = vboxsf_create_at_dentry(dentry, ¶ms);
That's... interesting. What should happen if you race with rename of
grandparent? Note that *parent* is locked here; no deeper ancestors
are.
The same goes for removals.
> +static const char *vboxsf_get_link(struct dentry *dentry, struct inode *inode,
> + struct delayed_call *done)
> +{
> + struct vboxsf_sbi *sbi = VBOXSF_SBI(inode->i_sb);
> + struct shfl_string *path;
> + char *link;
> + int err;
> +
> + if (!dentry)
> + return ERR_PTR(-ECHILD);
> +
> + path = vboxsf_path_from_dentry(sbi, dentry);
> + if (IS_ERR(path))
> + return (char *)path;
ERR_CAST(path)
> + /** No additional information is available / requested. */
> + SHFLFSOBJATTRADD_NOTHING = 1,
<unprintable>
Well, unpronounceable, actually...
> + switch (opt) {
> + case opt_nls:
> + if (fc->purpose != FS_CONTEXT_FOR_MOUNT) {
> + vbg_err("vboxsf: Cannot reconfigure nls option\n");
> + return -EINVAL;
> + }
> + ctx->nls_name = param->string;
> + param->string = NULL;
Umm... What happens if you are given several such? A leak?
> +{
> + int err;
> +
> + err = vboxsf_setup();
> + if (err)
> + return err;
> +
> + return vfs_get_super(fc, vfs_get_independent_super, vboxsf_fill_super);
return get_tree_nodev(fc, vboxsf_fill_super);
please,
> +static int vboxsf_reconfigure(struct fs_context *fc)
> +{
> + struct vboxsf_sbi *sbi = VBOXSF_SBI(fc->root->d_sb);
> + struct vboxsf_fs_context *ctx = fc->fs_private;
> + struct inode *iroot;
> +
> + iroot = ilookup(fc->root->d_sb, 0);
> + if (!iroot)
> + return -ENOENT;
Huh? If that's supposed to be root directory inode, what's wrong
with ->d_sb->s_root->d_inode?
> + path = dentry_path_raw(dentry, buf, PATH_MAX);
> + if (IS_ERR(path)) {
> + __putname(buf);
> + return (struct shfl_string *)path;
ERR_CAST(path)...
