From: apgo@patchbomb.org (Arthur Othieno)
To: Lin Feng Shen <shenlinf@cn.ibm.com>
Cc: neilb@cse.unsw.edu.au, nfs@lists.sourceforge.net,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] NFS: fix error handling on access_ok in compat_sys_nfsservctl
Date: Thu, 11 May 2006 23:29:13 -0400 [thread overview]
Message-ID: <20060512032913.GA9202@krypton> (raw)
In-Reply-To: <4462D816.7090207@cn.ibm.com>
On Thu, May 11, 2006 at 02:22:14PM +0800, Lin Feng Shen wrote:
> From: Lin Feng Shen <shenlinf@cn.ibm.com>
>
> Functions compat_nfs_svc_trans, compat_nfs_clnt_trans, compat_nfs_exp_trans,
> compat_nfs_getfd_trans and compat_nfs_getfs_trans, which are called by
> compat_sys_nfsservctl(fs/compat.c), don't handle the return value of
> access_ok properly. access_ok return 1 when the addr is valid, and 0
> when it's not, but these functions have the reversed understanding. When the
> address is valid, they always return -EFAULT to compat_sys_nfsservctl.
>
> An example is to run /usr/sbin/rpc.nfsd(32bit program on Power5). It doesn't
> function as expected. strace showes that nfsservctl returns -EFAULT.
>
> The patch fixes this by correcting the error handling on the return value of
> access_ok in the five functions. It is created against
> linux-2.6.17-rc3-git16.
>
> Signed-off-by: Lin Feng Shen <shenlinf@cn.ibm.com>
> ---
>
> --- linux-2.6.17-rc3-git16/fs/compat.c.orig 2006-05-09
> 09:07:38.000000000 -0500
> +++ linux-2.6.17-rc3-git16/fs/compat.c 2006-05-09 09:21:00.000000000
> -0500
> @@ -2032,101 +2032,96 @@ union compat_nfsctl_res {
>
> static int compat_nfs_svc_trans(struct nfsctl_arg *karg, struct
> compat_nfsctl_arg __user *arg)
Your client is happily munging whitespace all over:
patching file fs/compat.c
patch: **** malformed patch at line 25: compat_nfsctl_arg __user *arg)
> {
> - int err;
> -
> - err = access_ok(VERIFY_READ, &arg->ca32_svc, sizeof(arg->ca32_svc));
> - err |= get_user(karg->ca_version, &arg->ca32_version);
> - err |= __get_user(karg->ca_svc.svc_port, &arg->ca32_svc.svc32_port);
> - err |= __get_user(karg->ca_svc.svc_nthreads,
> &arg->ca32_svc.svc32_nthreads);
> - return (err) ? -EFAULT : 0;
> + if(!access_ok(VERIFY_READ, &arg->ca32_svc, sizeof(arg->ca32_svc)) ||
> + get_user(karg->ca_version, &arg->ca32_version) ||
> + __get_user(karg->ca_svc.svc_port, &arg->ca32_svc.svc32_port) ||
> + __get_user(karg->ca_svc.svc_nthreads,
> &arg->ca32_svc.svc32_nthreads))
> + return -EFAULT;
> + return 0;
> }
>
> static int compat_nfs_clnt_trans(struct nfsctl_arg *karg, struct
> compat_nfsctl_arg __user *arg)
> {
> - int err;
> -
> - err = access_ok(VERIFY_READ, &arg->ca32_client,
> sizeof(arg->ca32_client));
> - err |= get_user(karg->ca_version, &arg->ca32_version);
> - err |= __copy_from_user(&karg->ca_client.cl_ident[0],
> + if(!access_ok(VERIFY_READ, &arg->ca32_client,
> sizeof(arg->ca32_client)) ||
> + get_user(karg->ca_version, &arg->ca32_version) ||
> + __copy_from_user(&karg->ca_client.cl_ident[0],
> &arg->ca32_client.cl32_ident[0],
> - NFSCLNT_IDMAX);
> - err |= __get_user(karg->ca_client.cl_naddr,
> &arg->ca32_client.cl32_naddr);
> - err |= __copy_from_user(&karg->ca_client.cl_addrlist[0],
> + NFSCLNT_IDMAX) ||
> + __get_user(karg->ca_client.cl_naddr,
> &arg->ca32_client.cl32_naddr) ||
> + __copy_from_user(&karg->ca_client.cl_addrlist[0],
> &arg->ca32_client.cl32_addrlist[0],
> - (sizeof(struct in_addr) * NFSCLNT_ADDRMAX));
> - err |= __get_user(karg->ca_client.cl_fhkeytype,
> - &arg->ca32_client.cl32_fhkeytype);
> - err |= __get_user(karg->ca_client.cl_fhkeylen,
> - &arg->ca32_client.cl32_fhkeylen);
> - err |= __copy_from_user(&karg->ca_client.cl_fhkey[0],
> + (sizeof(struct in_addr) * NFSCLNT_ADDRMAX)) ||
> + __get_user(karg->ca_client.cl_fhkeytype,
> + &arg->ca32_client.cl32_fhkeytype) ||
> + __get_user(karg->ca_client.cl_fhkeylen,
> + &arg->ca32_client.cl32_fhkeylen) ||
> + __copy_from_user(&karg->ca_client.cl_fhkey[0],
> &arg->ca32_client.cl32_fhkey[0],
> - NFSCLNT_KEYMAX);
> + NFSCLNT_KEYMAX))
> + return -EFAULT;
>
> - return (err) ? -EFAULT : 0;
> + return 0;
> }
>
> static int compat_nfs_exp_trans(struct nfsctl_arg *karg, struct
> compat_nfsctl_arg __user *arg)
> {
> - int err;
> -
> - err = access_ok(VERIFY_READ, &arg->ca32_export,
> sizeof(arg->ca32_export));
> - err |= get_user(karg->ca_version, &arg->ca32_version);
> - err |= __copy_from_user(&karg->ca_export.ex_client[0],
> + if(!access_ok(VERIFY_READ, &arg->ca32_export,
> sizeof(arg->ca32_export)) ||
> + get_user(karg->ca_version, &arg->ca32_version) ||
> + __copy_from_user(&karg->ca_export.ex_client[0],
> &arg->ca32_export.ex32_client[0],
> - NFSCLNT_IDMAX);
> - err |= __copy_from_user(&karg->ca_export.ex_path[0],
> + NFSCLNT_IDMAX) ||
> + __copy_from_user(&karg->ca_export.ex_path[0],
> &arg->ca32_export.ex32_path[0],
> - NFS_MAXPATHLEN);
> - err |= __get_user(karg->ca_export.ex_dev,
> - &arg->ca32_export.ex32_dev);
> - err |= __get_user(karg->ca_export.ex_ino,
> - &arg->ca32_export.ex32_ino);
> - err |= __get_user(karg->ca_export.ex_flags,
> - &arg->ca32_export.ex32_flags);
> - err |= __get_user(karg->ca_export.ex_anon_uid,
> - &arg->ca32_export.ex32_anon_uid);
> - err |= __get_user(karg->ca_export.ex_anon_gid,
> - &arg->ca32_export.ex32_anon_gid);
> + NFS_MAXPATHLEN) ||
> + __get_user(karg->ca_export.ex_dev,
> + &arg->ca32_export.ex32_dev) ||
> + __get_user(karg->ca_export.ex_ino,
> + &arg->ca32_export.ex32_ino) ||
> + __get_user(karg->ca_export.ex_flags,
> + &arg->ca32_export.ex32_flags) ||
> + __get_user(karg->ca_export.ex_anon_uid,
> + &arg->ca32_export.ex32_anon_uid) ||
> + __get_user(karg->ca_export.ex_anon_gid,
> + &arg->ca32_export.ex32_anon_gid))
> + return -EFAULT;
> SET_UID(karg->ca_export.ex_anon_uid, karg->ca_export.ex_anon_uid);
> SET_GID(karg->ca_export.ex_anon_gid, karg->ca_export.ex_anon_gid);
>
> - return (err) ? -EFAULT : 0;
> + return 0;
> }
>
> static int compat_nfs_getfd_trans(struct nfsctl_arg *karg, struct
> compat_nfsctl_arg __user *arg)
> {
> - int err;
> -
> - err = access_ok(VERIFY_READ, &arg->ca32_getfd,
> sizeof(arg->ca32_getfd));
> - err |= get_user(karg->ca_version, &arg->ca32_version);
> - err |= __copy_from_user(&karg->ca_getfd.gd_addr,
> + if(!access_ok(VERIFY_READ, &arg->ca32_getfd,
> sizeof(arg->ca32_getfd)) ||
> + get_user(karg->ca_version, &arg->ca32_version) ||
> + __copy_from_user(&karg->ca_getfd.gd_addr,
> &arg->ca32_getfd.gd32_addr,
> - (sizeof(struct sockaddr)));
> - err |= __copy_from_user(&karg->ca_getfd.gd_path,
> + (sizeof(struct sockaddr))) ||
> + __copy_from_user(&karg->ca_getfd.gd_path,
> &arg->ca32_getfd.gd32_path,
> - (NFS_MAXPATHLEN+1));
> - err |= __get_user(karg->ca_getfd.gd_version,
> - &arg->ca32_getfd.gd32_version);
> + (NFS_MAXPATHLEN+1)) ||
> + __get_user(karg->ca_getfd.gd_version,
> + &arg->ca32_getfd.gd32_version))
> + return -EFAULT;
>
> - return (err) ? -EFAULT : 0;
> + return 0;
> }
>
> static int compat_nfs_getfs_trans(struct nfsctl_arg *karg, struct
> compat_nfsctl_arg __user *arg)
> {
> - int err;
> -
> - err = access_ok(VERIFY_READ, &arg->ca32_getfs,
> sizeof(arg->ca32_getfs));
> - err |= get_user(karg->ca_version, &arg->ca32_version);
> - err |= __copy_from_user(&karg->ca_getfs.gd_addr,
> + if(!access_ok(VERIFY_READ, &arg->ca32_getfs,
> sizeof(arg->ca32_getfs)) ||
> + get_user(karg->ca_version, &arg->ca32_version) ||
> + __copy_from_user(&karg->ca_getfs.gd_addr,
> &arg->ca32_getfs.gd32_addr,
> - (sizeof(struct sockaddr)));
> - err |= __copy_from_user(&karg->ca_getfs.gd_path,
> + (sizeof(struct sockaddr))) ||
> + __copy_from_user(&karg->ca_getfs.gd_path,
> &arg->ca32_getfs.gd32_path,
> - (NFS_MAXPATHLEN+1));
> - err |= __get_user(karg->ca_getfs.gd_maxlen,
> - &arg->ca32_getfs.gd32_maxlen);
> + (NFS_MAXPATHLEN+1)) ||
> + __get_user(karg->ca_getfs.gd_maxlen,
> + &arg->ca32_getfs.gd32_maxlen))
> + return -EFAULT;
>
> - return (err) ? -EFAULT : 0;
> + return 0;
> }
>
> /* This really doesn't need translations, we are only passing
next prev parent reply other threads:[~2006-05-12 3:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-05-11 6:22 [PATCH 1/1] NFS: fix error handling on access_ok in compat_sys_nfsservctl Lin Feng Shen
2006-05-11 6:22 ` Lin Feng Shen
2006-05-12 3:29 ` Arthur Othieno [this message]
-- strict thread matches above, loose matches on Subject: below --
2006-05-10 16:44 Lin Feng Shen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060512032913.GA9202@krypton \
--to=apgo@patchbomb.org \
--cc=linux-kernel@vger.kernel.org \
--cc=neilb@cse.unsw.edu.au \
--cc=nfs@lists.sourceforge.net \
--cc=shenlinf@cn.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.