From 9f415eb25574db4b73a9a712a4438e41dc284922 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Fri, 3 May 2013 16:09:09 -0400 Subject: nfsd4: don't allow owner override on 4.1 CLAIM_FH opens The Linux client is using CLAIM_FH to implement regular opens, not just recovery cases, so it depends on the server to check permissions correctly. Therefore the owner override, which may make sense in the delegation recovery case, isn't right in the CLAIM_FH case. Symptoms: on a client with 49f9a0fafd844c32f2abada047c0b9a5ba0d6255 "NFSv4.1: Enable open-by-filehandle", Bryan noticed this: touch test.txt chmod 000 test.txt echo test > test.txt succeeding. Cc: stable@kernel.org Reported-by: Bryan Schumaker Signed-off-by: J. Bruce Fields --- fs/nfsd/nfs4proc.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 8ae5abfe6ba..27d74a29451 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -279,6 +279,7 @@ do_open_fhandle(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, str { struct svc_fh *current_fh = &cstate->current_fh; __be32 status; + int accmode = 0; /* We don't know the target directory, and therefore can not * set the change info @@ -290,9 +291,19 @@ do_open_fhandle(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, str open->op_truncate = (open->op_iattr.ia_valid & ATTR_SIZE) && (open->op_iattr.ia_size == 0); + /* + * In the delegation case, the client is telling us about an + * open that it *already* performed locally, some time ago. We + * should let it succeed now if possible. + * + * In the case of a CLAIM_FH open, on the other hand, the client + * may be counting on us to enforce permissions (the Linux 4.1 + * client uses this for normal opens, for example). + */ + if (open->op_claim_type == NFS4_OPEN_CLAIM_DELEG_CUR_FH) + accmode = NFSD_MAY_OWNER_OVERRIDE; - status = do_open_permission(rqstp, current_fh, open, - NFSD_MAY_OWNER_OVERRIDE); + status = do_open_permission(rqstp, current_fh, open, accmode); return status; } -- cgit v1.2.3 From 9fd40c5a66be0b0a5d65362ec139968ba6d1f412 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Mon, 6 May 2013 09:21:03 +0200 Subject: SUNRPC: Refactor gssx_dec_option_array() to kill uninitialized warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit net/sunrpc/auth_gss/gss_rpc_xdr.c: In function ‘gssx_dec_option_array’: net/sunrpc/auth_gss/gss_rpc_xdr.c:258: warning: ‘creds’ may be used uninitialized in this function Return early if count is zero, to make it clearer to the compiler (and the casual reviewer) that no more processing is done. Signed-off-by: Geert Uytterhoeven Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_rpc_xdr.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c index 5c4c61d527e..a1e1b1ab515 100644 --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c @@ -264,25 +264,27 @@ static int gssx_dec_option_array(struct xdr_stream *xdr, if (unlikely(p == NULL)) return -ENOSPC; count = be32_to_cpup(p++); - if (count != 0) { - /* we recognize only 1 currently: CREDS_VALUE */ - oa->count = 1; + if (!count) + return 0; - oa->data = kmalloc(sizeof(struct gssx_option), GFP_KERNEL); - if (!oa->data) - return -ENOMEM; + /* we recognize only 1 currently: CREDS_VALUE */ + oa->count = 1; - creds = kmalloc(sizeof(struct svc_cred), GFP_KERNEL); - if (!creds) { - kfree(oa->data); - return -ENOMEM; - } + oa->data = kmalloc(sizeof(struct gssx_option), GFP_KERNEL); + if (!oa->data) + return -ENOMEM; - oa->data[0].option.data = CREDS_VALUE; - oa->data[0].option.len = sizeof(CREDS_VALUE); - oa->data[0].value.data = (void *)creds; - oa->data[0].value.len = 0; + creds = kmalloc(sizeof(struct svc_cred), GFP_KERNEL); + if (!creds) { + kfree(oa->data); + return -ENOMEM; } + + oa->data[0].option.data = CREDS_VALUE; + oa->data[0].option.len = sizeof(CREDS_VALUE); + oa->data[0].value.data = (void *)creds; + oa->data[0].value.len = 0; + for (i = 0; i < count; i++) { gssx_buffer dummy = { 0, NULL }; u32 length; -- cgit v1.2.3 From fb43f11c666a4f99f23f0be4fa528dcd288c0da2 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 7 May 2013 17:45:20 -0400 Subject: SUNRPC: fix decoding of optional gss-proxy xdr fields The current code works, but sort of by accident: it obviously didn't intend the error return to be interpreted as "true". Reported-by: Dan Carpenter Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_rpc_xdr.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c index a1e1b1ab515..357f613df7f 100644 --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c @@ -21,16 +21,6 @@ #include #include "gss_rpc_xdr.h" -static bool gssx_check_pointer(struct xdr_stream *xdr) -{ - __be32 *p; - - p = xdr_reserve_space(xdr, 4); - if (unlikely(p == NULL)) - return -ENOSPC; - return *p?true:false; -} - static int gssx_enc_bool(struct xdr_stream *xdr, int v) { __be32 *p; @@ -802,6 +792,7 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp, struct xdr_stream *xdr, struct gssx_res_accept_sec_context *res) { + u32 value_follows; int err; /* res->status */ @@ -810,7 +801,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp, return err; /* res->context_handle */ - if (gssx_check_pointer(xdr)) { + err = gssx_dec_bool(xdr, &value_follows); + if (err) + return err; + if (value_follows) { err = gssx_dec_ctx(xdr, res->context_handle); if (err) return err; @@ -819,7 +813,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp, } /* res->output_token */ - if (gssx_check_pointer(xdr)) { + err = gssx_dec_bool(xdr, &value_follows); + if (err) + return err; + if (value_follows) { err = gssx_dec_buffer(xdr, res->output_token); if (err) return err; @@ -828,7 +825,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp, } /* res->delegated_cred_handle */ - if (gssx_check_pointer(xdr)) { + err = gssx_dec_bool(xdr, &value_follows); + if (err) + return err; + if (value_follows) { /* we do not support upcall servers sending this data. */ return -EINVAL; } -- cgit v1.2.3 From 7255e716b1757dc10fa5e3a4d2eaab303ff9f7b6 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Thu, 9 May 2013 08:36:23 -0400 Subject: nfsd: fix oops when legacy_recdir_name_error is passed a -ENOENT error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toralf reported the following oops to the linux-nfs mailing list: -----------------[snip]------------------ NFSD: unable to generate recoverydir name (-2). NFSD: disabling legacy clientid tracking. Reboot recovery will not function correctly! BUG: unable to handle kernel NULL pointer dereference at 000003c8 IP: [] nfsd4_client_tracking_exit+0x11/0x50 [nfsd] *pdpt = 000000002ba33001 *pde = 0000000000000000 Oops: 0000 [#1] SMP Modules linked in: loop nfsd auth_rpcgss ipt_MASQUERADE xt_owner xt_multiport ipt_REJECT xt_tcpudp xt_recent xt_conntrack nf_conntrack_ftp xt_limit xt_LOG iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_filter ip_tables x_tables af_packet pppoe pppox ppp_generic slhc bridge stp llc tun arc4 iwldvm mac80211 coretemp kvm_intel uvcvideo sdhci_pci sdhci mmc_core videobuf2_vmalloc videobuf2_memops usblp videobuf2_core i915 iwlwifi psmouse videodev cfg80211 kvm fbcon bitblit cfbfillrect acpi_cpufreq mperf evdev softcursor font cfbimgblt i2c_algo_bit cfbcopyarea intel_agp intel_gtt drm_kms_helper snd_hda_codec_conexant drm agpgart fb fbdev tpm_tis thinkpad_acpi tpm nvram e1000e rfkill thermal ptp wmi pps_core tpm_bios 8250_pci processor 8250 ac snd_hda_intel snd_hda_codec snd_pcm battery video i2c_i801 snd_page_alloc snd_timer button serial_core i2c_core snd soundcore thermal_sys hwmon aesni_intel ablk_helper cryp td lrw aes_i586 xts gf128mul cbc fuse nfs lockd sunrpc dm_crypt dm_mod hid_monterey hid_microsoft hid_logitech hid_ezkey hid_cypress hid_chicony hid_cherry hid_belkin hid_apple hid_a4tech hid_generic usbhid hid sr_mod cdrom sg [last unloaded: microcode] Pid: 6374, comm: nfsd Not tainted 3.9.1 #6 LENOVO 4180F65/4180F65 EIP: 0060:[] EFLAGS: 00010202 CPU: 0 EIP is at nfsd4_client_tracking_exit+0x11/0x50 [nfsd] EAX: 00000000 EBX: fffffffe ECX: 00000007 EDX: 00000007 ESI: eb9dcb00 EDI: eb2991c0 EBP: eb2bde38 ESP: eb2bde34 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 CR0: 80050033 CR2: 000003c8 CR3: 2ba80000 CR4: 000407f0 DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 DR6: ffff0ff0 DR7: 00000400 Process nfsd (pid: 6374, ti=eb2bc000 task=eb2711c0 task.ti=eb2bc000) Stack: fffffffe eb2bde4c f90a3e0c f90a7754 fffffffe eb0a9c00 eb2bdea0 f90a41ed eb2991c0 1b270000 eb2991c0 eb2bde7c f9099ce9 eb2bde98 0129a020 eb29a020 eb2bdecc eb2991c0 eb2bdea8 f9099da5 00000000 eb9dcb00 00000001 67822f08 Call Trace: [] legacy_recdir_name_error+0x3c/0x40 [nfsd] [] nfsd4_create_clid_dir+0x15d/0x1c0 [nfsd] [] ? nfsd4_lookup_stateid+0x99/0xd0 [nfsd] [] ? nfs4_preprocess_seqid_op+0x85/0x100 [nfsd] [] nfsd4_client_record_create+0x37/0x50 [nfsd] [] nfsd4_open_confirm+0xfe/0x130 [nfsd] [] ? nfsd4_encode_operation+0x61/0x90 [nfsd] [] ? nfsd4_free_stateid+0xc0/0xc0 [nfsd] [] nfsd4_proc_compound+0x41b/0x530 [nfsd] [] nfsd_dispatch+0x8b/0x1a0 [nfsd] [] svc_process+0x3dd/0x640 [sunrpc] [] nfsd+0xad/0x110 [nfsd] [] ? nfsd_destroy+0x70/0x70 [nfsd] [] kthread+0x94/0xa0 [] ret_from_kernel_thread+0x1b/0x28 [] ? flush_kthread_work+0xd0/0xd0 Code: 86 b0 00 00 00 90 c5 0a f9 c7 04 24 70 76 0a f9 e8 74 a9 3d c8 eb ba 8d 76 00 55 89 e5 53 66 66 66 66 90 8b 15 68 c7 0a f9 85 d2 <8b> 88 c8 03 00 00 74 2c 3b 11 77 28 8b 5c 91 08 85 db 74 22 8b EIP: [] nfsd4_client_tracking_exit+0x11/0x50 [nfsd] SS:ESP 0068:eb2bde34 CR2: 00000000000003c8 ---[ end trace 09e54015d145c9c6 ]--- The problem appears to be a regression that was introduced in commit 9a9c6478 "nfsd: make NFSv4 recovery client tracking options per net". Prior to that commit, it was safe to pass a NULL net pointer to nfsd4_client_tracking_exit in the legacy recdir case, and legacy_recdir_name_error did so. After that comit, the net pointer must be valid. This patch just fixes legacy_recdir_name_error to pass in a valid net pointer to that function. Cc: # v3.8+ Cc: Stanislav Kinsbursky Reported-and-tested-by: Toralf Förster Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- fs/nfsd/nfs4recover.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index 899ca26dd19..4e9a21db867 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -146,7 +146,7 @@ out_no_tfm: * then disable recovery tracking. */ static void -legacy_recdir_name_error(int error) +legacy_recdir_name_error(struct nfs4_client *clp, int error) { printk(KERN_ERR "NFSD: unable to generate recoverydir " "name (%d).\n", error); @@ -159,9 +159,7 @@ legacy_recdir_name_error(int error) if (error == -ENOENT) { printk(KERN_ERR "NFSD: disabling legacy clientid tracking. " "Reboot recovery will not function correctly!\n"); - - /* the argument is ignored by the legacy exit function */ - nfsd4_client_tracking_exit(NULL); + nfsd4_client_tracking_exit(clp->net); } } @@ -184,7 +182,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp) status = nfs4_make_rec_clidname(dname, &clp->cl_name); if (status) - return legacy_recdir_name_error(status); + return legacy_recdir_name_error(clp, status); status = nfs4_save_creds(&original_cred); if (status < 0) @@ -341,7 +339,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp) status = nfs4_make_rec_clidname(dname, &clp->cl_name); if (status) - return legacy_recdir_name_error(status); + return legacy_recdir_name_error(clp, status); status = mnt_want_write_file(nn->rec_file); if (status) @@ -601,7 +599,7 @@ nfsd4_check_legacy_client(struct nfs4_client *clp) status = nfs4_make_rec_clidname(dname, &clp->cl_name); if (status) { - legacy_recdir_name_error(status); + legacy_recdir_name_error(clp, status); return status; } -- cgit v1.2.3