* [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() @ 2021-08-24 11:48 Dan Carpenter 2021-08-24 16:38 ` Kari Argillander 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2021-08-24 11:48 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, kernel-janitors The ntfs_get_ea() function returns negative error codes or on success it returns the length. In the original code a zero length return was treated as -ENODATA and results in a NULL return. But it should be treated as an invalid length and result in an PTR_ERR(-EINVAL) return. Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- I'm not super familiar with this code. Please review this one extra carefully. I think it's theoretical because hopefully ntfs_get_ea() doesn't ever return invalid lengths. fs/ntfs3/xattr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c index 9239c388050e..e8ed38d0c4c9 100644 --- a/fs/ntfs3/xattr.c +++ b/fs/ntfs3/xattr.c @@ -521,7 +521,7 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, ni_unlock(ni); /* Translate extended attribute to acl */ - if (err > 0) { + if (err >= 0) { acl = posix_acl_from_xattr(mnt_userns, buf, err); if (!IS_ERR(acl)) set_cached_acl(inode, type, acl); -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() 2021-08-24 11:48 [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() Dan Carpenter @ 2021-08-24 16:38 ` Kari Argillander 2021-08-24 17:07 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Kari Argillander @ 2021-08-24 16:38 UTC (permalink / raw) To: Dan Carpenter; +Cc: Konstantin Komarov, ntfs3, linux-kernel, kernel-janitors On Tue, Aug 24, 2021 at 02:48:58PM +0300, Dan Carpenter wrote: > The ntfs_get_ea() function returns negative error codes or on success Not reletad to this patch but ntfs_get_wsl_perm() seems quite bug because in there ntfs_get_ea use is not checked at all. Also ntfs_getxattr() should probably send errno if ntfs_get_ea() is 0. > it returns the length. In the original code a zero length return was > treated as -ENODATA and results in a NULL return. But it should be > treated as an invalid length and result in an PTR_ERR(-EINVAL) return. > > Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > I'm not super familiar with this code. Please review this one > extra carefully. I think it's theoretical because hopefully > ntfs_get_ea() doesn't ever return invalid lengths. ntfs_get_ea() will return 0 if no info and this can happend quite easily in my eyes. Here's snippets ntfs_read_ea() { attr_info = ni_find_attr(ni, NULL, &le, ATTR_EA_INFO, NULL, 0, NULL, NULL); attr_ea = ni_find_attr(ni, attr_info, &le, ATTR_EA, NULL, 0, NULL, NULL); if (!attr_ea || !attr_info) return 0; } ntfs_get_ea() { len = 0; err = ntfs_read_ea(ni, &ea_all, 0, &info); if (err) goto out; if (!info) goto out; out: return err ? err : len; } > > fs/ntfs3/xattr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > index 9239c388050e..e8ed38d0c4c9 100644 > --- a/fs/ntfs3/xattr.c > +++ b/fs/ntfs3/xattr.c > @@ -521,7 +521,7 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, > ni_unlock(ni); > > /* Translate extended attribute to acl */ > - if (err > 0) { > + if (err >= 0) { So now if err (size) is 0 it will try to get acl. Didn't you just say that you want to return PTR_ERR(-EINVAL)? So overall good finding but maybe more work is needed with this one. > acl = posix_acl_from_xattr(mnt_userns, buf, err); > if (!IS_ERR(acl)) > set_cached_acl(inode, type, acl); > -- > 2.20.1 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() 2021-08-24 16:38 ` Kari Argillander @ 2021-08-24 17:07 ` Dan Carpenter 2021-08-27 17:17 ` Konstantin Komarov 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2021-08-24 17:07 UTC (permalink / raw) To: Kari Argillander; +Cc: Konstantin Komarov, ntfs3, linux-kernel, kernel-janitors On Tue, Aug 24, 2021 at 07:38:51PM +0300, Kari Argillander wrote: > On Tue, Aug 24, 2021 at 02:48:58PM +0300, Dan Carpenter wrote: > > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > > index 9239c388050e..e8ed38d0c4c9 100644 > > --- a/fs/ntfs3/xattr.c > > +++ b/fs/ntfs3/xattr.c > > @@ -521,7 +521,7 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, > > ni_unlock(ni); > > > > /* Translate extended attribute to acl */ > > - if (err > 0) { > > + if (err >= 0) { > > So now if err (size) is 0 it will try to get acl. Didn't you just say > that you want to return PTR_ERR(-EINVAL)? > If you pass an invalid too short size to posix_acl_from_xattr() then it returns PTR_ERR(-EINVAL). It was hard to phrase this in the change log but I feel like length of 1 and 0 should be treated the same. > So overall good finding but maybe more work is needed with this one. > > > acl = posix_acl_from_xattr(mnt_userns, buf, err); > > if (!IS_ERR(acl)) > > set_cached_acl(inode, type, acl); regards, dan carpenter ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() 2021-08-24 17:07 ` Dan Carpenter @ 2021-08-27 17:17 ` Konstantin Komarov 0 siblings, 0 replies; 4+ messages in thread From: Konstantin Komarov @ 2021-08-27 17:17 UTC (permalink / raw) To: Dan Carpenter, Kari Argillander; +Cc: ntfs3, linux-kernel, kernel-janitors > From: Dan Carpenter <dan.carpenter@oracle.com> > Sent: Tuesday, August 24, 2021 8:07 PM > To: Kari Argillander <kari.argillander@gmail.com> > Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>; ntfs3@lists.linux.dev; linux-kernel@vger.kernel.org; kernel- > janitors@vger.kernel.org > Subject: Re: [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() > > On Tue, Aug 24, 2021 at 07:38:51PM +0300, Kari Argillander wrote: > > On Tue, Aug 24, 2021 at 02:48:58PM +0300, Dan Carpenter wrote: > > > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > > > index 9239c388050e..e8ed38d0c4c9 100644 > > > --- a/fs/ntfs3/xattr.c > > > +++ b/fs/ntfs3/xattr.c > > > @@ -521,7 +521,7 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, > > > ni_unlock(ni); > > > > > > /* Translate extended attribute to acl */ > > > - if (err > 0) { > > > + if (err >= 0) { > > > > So now if err (size) is 0 it will try to get acl. Didn't you just say > > that you want to return PTR_ERR(-EINVAL)? > > > > If you pass an invalid too short size to posix_acl_from_xattr() then it > returns PTR_ERR(-EINVAL). It was hard to phrase this in the change log > but I feel like length of 1 and 0 should be treated the same. > > > > So overall good finding but maybe more work is needed with this one. > > > > > acl = posix_acl_from_xattr(mnt_userns, buf, err); > > > if (!IS_ERR(acl)) > > > set_cached_acl(inode, type, acl); > > regards, > dan carpenter > Applied, thanks! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-27 17:18 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-24 11:48 [PATCH] fs/ntfs3: fix an error code in ntfs_get_acl_ex() Dan Carpenter 2021-08-24 16:38 ` Kari Argillander 2021-08-24 17:07 ` Dan Carpenter 2021-08-27 17:17 ` Konstantin Komarov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).