| 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
| ||
|
Date: Tue, 22 Oct 2013 08:35:09 -0200
From: Geyslan Gregório Bem <geyslan@...il.com>
To: Eric Van Hensbergen <ericvh@...il.com>
Cc: Joe Perches <joe@...ches.com>, Ron Minnich <rminnich@...dia.gov>,
Latchesar Ionkov <lucho@...kov.net>,
V9FS Developers <v9fs-developer@...ts.sourceforge.net>,
linux-kernel <linux-kernel@...r.kernel.org>,
kernel-br <kernel-br@...glegroups.com>
Subject: Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/21 Geyslan Gregório Bem <geyslan@...il.com>:
> 2013/10/21 Geyslan Gregório Bem <geyslan@...il.com>:
>> 2013/10/20 Eric Van Hensbergen <ericvh@...il.com>:
>>> Please resubmit a clean patch which includes the check of sscanf for exactly
>>> the correct number of arguments and handles errors properly in other cases.
>>> That last bit may be a bit problematic since right now the only errors are
>>> prints and we seem to be otherwise silently failing. Of course, looks like
>>> nothing else is checking return values from that function for error. We
>>> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
>>> all the places which matter (looks like there are a few places where rdev
>>> just gets discarded -- might even be a good idea to not parse rdev unless we
>>> need to by passing NULL to p9mode2unixmode.
>>>
>>> All in all, its a corner case which is only likely with a broken server, but
>>> the full clean up would seem to be:
>>> a) switch to u32's
>>> b) pass NULL when rdev just gets discarded and don't bother parsing when
>>> it is
>>> c) check the sscanf return validity
>>> d) on error set ERR_PTR in rdev and check on return before proceeding
>>>
>>> That's a lot of cleanup, I'll add it to my work queue if you don't have time
>>> to rework your patch.
>>>
>>
>> Eric, I would like to try with your guidance.
>>
>>> For the other patches, anyone you didn't see a response from me on today is
>>> being pulled into my for-next queue. Thanks for the cleanups.
>>>
>>> -eric
>>
>> Thanks for accept them.
>>
>>>
>>>
>>>
>>>
>>> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@...il.com>
>>> wrote:
>>>>
>>>> Joe,
>>>>
>>>> Nice, I'll wait their reply, there are other p9 patches that I have
>>>> already sent and am awaiting Eric's.
>>>>
>>>> Thank you again.
>>>>
>>>> Geyslan Gregório Bem
>>>> hackingbits.com
>>>>
>
> Let me know if I got your requests:
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 94de6d1..8a332d0 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -63,7 +63,7 @@ static const struct inode_operations
> v9fs_symlink_inode_operations;
>
> static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
> {
> - int res;
> + u32 res;
> res = mode & 0777;
> if (S_ISDIR(mode))
> res |= P9_DMDIR;
> @@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
> static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
> struct p9_wstat *stat, dev_t *rdev)
> {
> - int res;
> + umode_t res;
> u32 mode = stat->mode;
>
> *rdev = 0;
> @@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
> else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
> && (v9ses->nodev == 0)) {
> char type = 0, ext[32];
> - int major = -1, minor = -1;
> + u32 major = 0, minor = 0;
>
> strlcpy(ext, stat->extension, sizeof(ext));
> - sscanf(ext, "%c %u %u", &type, &major, &minor);
> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
> + p9_debug(P9_DEBUG_ERROR,
> + "It's necessary define type [%u], major [u%] and minor [u%]" \
> + "values when using P9_DMDEVICE", type, major, minor);
> + goto err_dev;
> + }
> switch (type) {
> case 'c':
> res |= S_IFCHR;
> @@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
> default:
> p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
> type, stat->extension);
> + goto err_dev;
> };
> *rdev = MKDEV(major, minor);
> } else
> res |= S_IFREG;
> + goto ret;
>
> +err_dev:
> + rdev = ERR_PTR(-EIO);
> +ret:
> return res;
> }
>
> @@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)
>
> static int v9fs_test_inode(struct inode *inode, void *data)
> {
> - int umode;
> + umode_t umode;
> dev_t rdev;
> struct v9fs_inode *v9inode = V9FS_I(inode);
> struct p9_wstat *st = (struct p9_wstat *)data;
> struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
>
> umode = p9mode2unixmode(v9ses, st, &rdev);
> +
> + if (IS_ERR(rdev))
> + return 0;
> +
> /* don't match inode of different type */
> if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
> return 0;
> @@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
> */
> inode->i_ino = i_ino;
> umode = p9mode2unixmode(v9ses, st, &rdev);
> + if (IS_ERR(rdev)) {
> + retval = rdev;
> + goto error;
> + }
> +
> retval = v9fs_init_inode(v9ses, inode, umode, rdev);
> if (retval)
> goto error;
> @@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
> *dentry, umode_t mode, dev_t rde
>
> int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
> {
> - int umode;
> + umode_t umode;
> dev_t rdev;
> loff_t i_size;
> + int ret = 0;
> struct p9_wstat *st;
> struct v9fs_session_info *v9ses;
>
> @@ -1475,6 +1495,11 @@ int v9fs_refresh_inode(struct p9_fid *fid,
> struct inode *inode)
> * Don't update inode if the file type is different
> */
> umode = p9mode2unixmode(v9ses, st, &rdev);
> + if (IS_ERR(rdev)) {
> + ret = rdev;
> + goto out;
> + }
> +
> if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
> goto out;
>
> @@ -1491,7 +1516,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
> struct inode *inode)
> out:
> p9stat_free(st);
> kfree(st);
> - return 0;
> + return ret;
> }
>
> static const struct inode_operations v9fs_dir_inode_operations_dotu = {
>
I didn't compile it. It's just a sketch.
The rdev tests (in the callers) must be:
if (rdev < 0) {
...
Waiting your reply.
Cheers.
>
>>>>
>>>> 2013/10/7 Joe Perches <joe@...ches.com>:
>>>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>>>> >> Joe,
>>>> >>
>>>> >> Thank you for reply.
>>>> >>
>>>> >> What do you think about:
>>>> >>
>>>> >> strncpy(ext, stat->extension, sizeof(ext));
>>>> >> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) <
>>>> >> 3) {
>>>> >> + p9_debug(P9_DEBUG_ERROR,
>>>> >> + "It's necessary define type, major
>>>> >> and minor values when using P9_DMDEVICE");
>>>> >> + return res;
>>>> >> + }
>>>> >> switch (type) {
>>>> >> case 'c':
>>>> >> res |= S_IFCHR;
>>>> >> break;
>>>> >> ...
>>>> >> *rdev = MKDEV(major, minor);
>>>> >
>>>> > I think the plan 9 folk should figure out what's right.
>>>> >
>>>> >
>>>
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists