linux-kernel - RE: [PATCH v2] 9p: retrieve fid from file when file instance exist.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Jul 2020 01:09:49 +0000
From:   Jianyong Wu <Jianyong.Wu@....com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
CC:     "ericvh@...il.com" <ericvh@...il.com>,
        "lucho@...kov.net" <lucho@...kov.net>,
        "v9fs-developer@...ts.sourceforge.net" 
        <v9fs-developer@...ts.sourceforge.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Steve Capper <Steve.Capper@....com>,
        Kaly Xin <Kaly.Xin@....com>, Justin He <Justin.He@....com>,
        Wei Chen <Wei.Chen@....com>
Subject: RE: [PATCH v2] 9p: retrieve fid from file when file instance exist.

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@...ewreck.org>
> Sent: Wednesday, July 1, 2020 6:59 PM
> To: Jianyong Wu <Jianyong.Wu@....com>
> Cc: ericvh@...il.com; lucho@...kov.net; v9fs-
> developer@...ts.sourceforge.net; linux-kernel@...r.kernel.org; Steve
> Capper <Steve.Capper@....com>; Kaly Xin <Kaly.Xin@....com>; Justin He
> <Justin.He@....com>; Wei Chen <Wei.Chen@....com>
> Subject: Re: [PATCH v2] 9p: retrieve fid from file when file instance exist.
>
> Jianyong Wu wrote on Wed, Jul 01, 2020:
> > In the current setattr implementation in 9p, fid is always retrieved
> > from dentry no matter file instance exists or not. There may be some
> > info related to opened file instance dropped. so it's better to
> > retrieve fid from file instance if file instance is passed to setattr.
> >
> > for example:
> > fd=open("tmp", O_RDWR);
> > ftruncate(fd, 10);
> >
> > The file context related with fd will be lost as fid is always
> > retrieved from dentry, then the backend can't get the info of file
> > context. It is against the original intention of user and may lead to
> > bug.
>
> Thanks for the commit message - still feels a bit odd but at least correct
> enough for me :)
>
Thanks.

> > Signed-off-by: Jianyong Wu <jianyong.wu@....com>
> > ---
> >  fs/9p/vfs_inode.c      | 6 +++++-
> >  fs/9p/vfs_inode_dotl.c | 6 +++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index
> > c9255d399917..b33574d347fa 100644
> > --- a/fs/9p/vfs_inode.c
> > +++ b/fs/9p/vfs_inode.c
> > @@ -1100,7 +1100,11 @@ static int v9fs_vfs_setattr(struct dentry
> > *dentry, struct iattr *iattr)
> >
> >  retval = -EPERM;
> >  v9ses = v9fs_dentry2v9ses(dentry);
> > -fid = v9fs_fid_lookup(dentry);
> > +if (iattr->ia_valid & ATTR_FILE) {
> > +fid = iattr->ia_file->private_data;
> > +WARN_ON(!fid);
>
> That would crash in 9p_client_wstat a few lines below with the current else ;
> so I'm not sure WARN_ON is appropriate with this code.
>
> the snippet I had suggested had v9fs_fid_lookup in a different if, not as a else
> statement to avoid this crash (and then warning is OK)
>
Yeah, should check fid before "v9fs_fid_lookup", how about

if (iattr->ia_valid & ATTR_FILE) {
                fid = iattr->ia_file->private_data;
                WARN_ON(!fid);
 }
If (!fid)
fid = v9fs_fid_lookup(dentry);
...

Thanks
Jianyong

> > +} else
> > +fid = v9fs_fid_lookup(dentry);
>
> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.