| Message ID | 20240502195800.3252-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] nfsd: set security label during create operations | expand |
On Thu, May 02, 2024 at 03:58:01PM -0400, Stephen Smalley wrote: > When security labeling is enabled, the client can pass a file security > label as part of a create operation for the new file, similar to mode > and other attributes. At present, the security label is received by nfsd > and passed down to nfsd_create_setattr(), but nfsd_setattr() is never > called and therefore the label is never set on the new file. I couldn't > tell if this has always been broken or broke at some point in time. Might have been introduced on or around commit d6a97d3f589a ("NFSD: add security label to struct nfsd_attrs"). Neil, can you spare an eyeball or two for Stephen's patch? > Looking > at nfsd_setattr() I am uncertain as to whether the same issue presents for > file ACLs and therefore requires a similar fix for those. I am not overly > confident that this is the right solution. > > An alternative approach would be to introduce a new LSM hook to set the > "create SID" of the current task prior to the actual file creation, which > would atomically label the new inode at creation time. This would be better > for SELinux and a similar approach has been used previously > (see security_dentry_create_files_as) but perhaps not usable by other LSMs. > > Reproducer: > 1. Install a Linux distro with SELinux - Fedora is easiest > 2. git clone https://github.com/SELinuxProject/selinux-testsuite > 3. Install the requisite dependencies per selinux-testsuite/README.md > 4. Run something like the following script: > MOUNT=$HOME/selinux-testsuite > sudo systemctl start nfs-server > sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT > sudo mkdir -p /mnt/selinux-testsuite > sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite > pushd /mnt/selinux-testsuite/ > sudo make -C policy load > pushd tests/filesystem > sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \ > -e test_filesystem_filetranscon_t -v > sudo rm -f trans_test_file > popd > sudo make -C policy unload > popd > sudo umount /mnt/selinux-testsuite > sudo exportfs -u localhost:$MOUNT > sudo rmdir /mnt/selinux-testsuite > sudo systemctl stop nfs-server > > Expected output: > <eliding noise from commands run prior to or after the test itself> > Process context: > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > Created file: trans_test_file > File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0 > File context is correct > > Actual output: > <eliding noise from commands run prior to or after the test itself> > Process context: > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > Created file: trans_test_file > File context: system_u:object_r:test_file_t:s0 > File context error, expected: > test_filesystem_filetranscon_t > got: > test_file_t > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by > Jeffrey Layton <jlayton@kernel.org>. > > fs/nfsd/nfsproc.c | 2 +- > fs/nfsd/vfs.c | 2 +- > fs/nfsd/vfs.h | 8 ++++++++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 36370b957b63..3e438159f561 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > * open(..., O_CREAT|O_TRUNC|O_WRONLY). > */ > attr->ia_valid &= ATTR_SIZE; > - if (attr->ia_valid) > + if (nfsd_attrs_valid(attr)) > resp->status = nfsd_setattr(rqstp, newfhp, &attrs, > NULL); > } > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2e41eb4c3cec..29b1f3613800 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1422,7 +1422,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > * Callers expect new file metadata to be committed even > * if the attributes have not changed. > */ > - if (iap->ia_valid) > + if (nfsd_attrs_valid(attrs)) > status = nfsd_setattr(rqstp, resfhp, attrs, NULL); > else > status = nfserrno(commit_metadata(resfhp)); > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index c60fdb6200fd..57cd70062048 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -60,6 +60,14 @@ static inline void nfsd_attrs_free(struct nfsd_attrs *attrs) > posix_acl_release(attrs->na_dpacl); > } > > +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs) > +{ > + struct iattr *iap = attrs->na_iattr; > + > + return (iap->ia_valid || (attrs->na_seclabel && > + attrs->na_seclabel->len)); > +} > + > __be32 nfserrno (int errno); > int nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp, > struct svc_export **expp); > -- > 2.40.1 >
On Thu, 2024-05-02 at 15:58 -0400, Stephen Smalley wrote: > When security labeling is enabled, the client can pass a file security > label as part of a create operation for the new file, similar to mode > and other attributes. At present, the security label is received by nfsd > and passed down to nfsd_create_setattr(), but nfsd_setattr() is never > called and therefore the label is never set on the new file. I couldn't > tell if this has always been broken or broke at some point in time. Looking > at nfsd_setattr() I am uncertain as to whether the same issue presents for > file ACLs and therefore requires a similar fix for those. I am not overly > confident that this is the right solution. > > An alternative approach would be to introduce a new LSM hook to set the > "create SID" of the current task prior to the actual file creation, which > would atomically label the new inode at creation time. This would be better > for SELinux and a similar approach has been used previously > (see security_dentry_create_files_as) but perhaps not usable by other LSMs. > > Reproducer: > 1. Install a Linux distro with SELinux - Fedora is easiest > 2. git clone https://github.com/SELinuxProject/selinux-testsuite > 3. Install the requisite dependencies per selinux-testsuite/README.md > 4. Run something like the following script: > MOUNT=$HOME/selinux-testsuite > sudo systemctl start nfs-server > sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT > sudo mkdir -p /mnt/selinux-testsuite > sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite > pushd /mnt/selinux-testsuite/ > sudo make -C policy load > pushd tests/filesystem > sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \ > -e test_filesystem_filetranscon_t -v > sudo rm -f trans_test_file > popd > sudo make -C policy unload > popd > sudo umount /mnt/selinux-testsuite > sudo exportfs -u localhost:$MOUNT > sudo rmdir /mnt/selinux-testsuite > sudo systemctl stop nfs-server > > Expected output: > <eliding noise from commands run prior to or after the test itself> > Process context: > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > Created file: trans_test_file > File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0 > File context is correct > > Actual output: > <eliding noise from commands run prior to or after the test itself> > Process context: > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > Created file: trans_test_file > File context: system_u:object_r:test_file_t:s0 > File context error, expected: > test_filesystem_filetranscon_t > got: > test_file_t > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by > Jeffrey Layton <jlayton@kernel.org>. > > fs/nfsd/nfsproc.c | 2 +- > fs/nfsd/vfs.c | 2 +- > fs/nfsd/vfs.h | 8 ++++++++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 36370b957b63..3e438159f561 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > * open(..., O_CREAT|O_TRUNC|O_WRONLY). > */ > attr->ia_valid &= ATTR_SIZE; > - if (attr->ia_valid) > + if (nfsd_attrs_valid(attr)) > resp->status = nfsd_setattr(rqstp, newfhp, &attrs, > NULL); > } This function is for NFSv2, which doesn't support any inode attributes that aren't represented in ia_valid. We could leave this as-is, but this is fine too. > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2e41eb4c3cec..29b1f3613800 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1422,7 +1422,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > * Callers expect new file metadata to be committed even > * if the attributes have not changed. > */ > - if (iap->ia_valid) > + if (nfsd_attrs_valid(attrs)) > status = nfsd_setattr(rqstp, resfhp, attrs, NULL); > else > status = nfserrno(commit_metadata(resfhp)); > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index c60fdb6200fd..57cd70062048 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -60,6 +60,14 @@ static inline void nfsd_attrs_free(struct nfsd_attrs *attrs) > posix_acl_release(attrs->na_dpacl); > } > > +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs) > +{ > + struct iattr *iap = attrs->na_iattr; > + > + return (iap->ia_valid || (attrs->na_seclabel && > + attrs->na_seclabel->len)); > +} > + > __be32 nfserrno (int errno); > int nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp, > struct svc_export **expp); Reviewed-by: Jeffrey Layton <jlayton@kernel.org>
Hi Stephen,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.9-rc6 next-20240502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stephen-Smalley/nfsd-set-security-label-during-create-operations/20240503-040242
base: linus/master
patch link: https://lore.kernel.org/r/20240502195800.3252-1-stephen.smalley.work%40gmail.com
patch subject: [PATCH v2] nfsd: set security label during create operations
config: arm64-randconfig-r123-20240503 (https://download.01.org/0day-ci/archive/20240503/202405031516.kghPPWFt-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 37ae4ad0eef338776c7e2cffb3896153d43dcd90)
reproduce: (https://download.01.org/0day-ci/archive/20240503/202405031516.kghPPWFt-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405031516.kghPPWFt-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from fs/nfsd/nfsproc.c:10:
In file included from fs/nfsd/cache.h:12:
In file included from include/linux/sunrpc/svc.h:17:
In file included from include/linux/sunrpc/xdr.h:17:
In file included from include/linux/scatterlist.h:8:
In file included from include/linux/mm.h:2210:
include/linux/vmstat.h:508:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
508 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
509 | item];
| ~~~~
include/linux/vmstat.h:515:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
515 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
516 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:527:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
527 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
528 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:536:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
536 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
537 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> fs/nfsd/nfsproc.c:392:24: error: incompatible pointer types passing 'struct iattr *' to parameter of type 'struct nfsd_attrs *' [-Werror,-Wincompatible-pointer-types]
392 | if (nfsd_attrs_valid(attr))
| ^~~~
fs/nfsd/vfs.h:63:56: note: passing argument to parameter 'attrs' here
63 | static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
| ^
5 warnings and 1 error generated.
vim +392 fs/nfsd/nfsproc.c
240
241 /*
242 * CREATE processing is complicated. The keyword here is `overloaded.'
243 * The parent directory is kept locked between the check for existence
244 * and the actual create() call in compliance with VFS protocols.
245 * N.B. After this call _both_ argp->fh and resp->fh need an fh_put
246 */
247 static __be32
248 nfsd_proc_create(struct svc_rqst *rqstp)
249 {
250 struct nfsd_createargs *argp = rqstp->rq_argp;
251 struct nfsd_diropres *resp = rqstp->rq_resp;
252 svc_fh *dirfhp = &argp->fh;
253 svc_fh *newfhp = &resp->fh;
254 struct iattr *attr = &argp->attrs;
255 struct nfsd_attrs attrs = {
256 .na_iattr = attr,
257 };
258 struct inode *inode;
259 struct dentry *dchild;
260 int type, mode;
261 int hosterr;
262 dev_t rdev = 0, wanted = new_decode_dev(attr->ia_size);
263
264 dprintk("nfsd: CREATE %s %.*s\n",
265 SVCFH_fmt(dirfhp), argp->len, argp->name);
266
267 /* First verify the parent file handle */
268 resp->status = fh_verify(rqstp, dirfhp, S_IFDIR, NFSD_MAY_EXEC);
269 if (resp->status != nfs_ok)
270 goto done; /* must fh_put dirfhp even on error */
271
272 /* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
273
274 resp->status = nfserr_exist;
275 if (isdotent(argp->name, argp->len))
276 goto done;
277 hosterr = fh_want_write(dirfhp);
278 if (hosterr) {
279 resp->status = nfserrno(hosterr);
280 goto done;
281 }
282
283 inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
284 dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
285 if (IS_ERR(dchild)) {
286 resp->status = nfserrno(PTR_ERR(dchild));
287 goto out_unlock;
288 }
289 fh_init(newfhp, NFS_FHSIZE);
290 resp->status = fh_compose(newfhp, dirfhp->fh_export, dchild, dirfhp);
291 if (!resp->status && d_really_is_negative(dchild))
292 resp->status = nfserr_noent;
293 dput(dchild);
294 if (resp->status) {
295 if (resp->status != nfserr_noent)
296 goto out_unlock;
297 /*
298 * If the new file handle wasn't verified, we can't tell
299 * whether the file exists or not. Time to bail ...
300 */
301 resp->status = nfserr_acces;
302 if (!newfhp->fh_dentry) {
303 printk(KERN_WARNING
304 "nfsd_proc_create: file handle not verified\n");
305 goto out_unlock;
306 }
307 }
308
309 inode = d_inode(newfhp->fh_dentry);
310
311 /* Unfudge the mode bits */
312 if (attr->ia_valid & ATTR_MODE) {
313 type = attr->ia_mode & S_IFMT;
314 mode = attr->ia_mode & ~S_IFMT;
315 if (!type) {
316 /* no type, so if target exists, assume same as that,
317 * else assume a file */
318 if (inode) {
319 type = inode->i_mode & S_IFMT;
320 switch(type) {
321 case S_IFCHR:
322 case S_IFBLK:
323 /* reserve rdev for later checking */
324 rdev = inode->i_rdev;
325 attr->ia_valid |= ATTR_SIZE;
326
327 fallthrough;
328 case S_IFIFO:
329 /* this is probably a permission check..
330 * at least IRIX implements perm checking on
331 * echo thing > device-special-file-or-pipe
332 * by doing a CREATE with type==0
333 */
334 resp->status = nfsd_permission(rqstp,
335 newfhp->fh_export,
336 newfhp->fh_dentry,
337 NFSD_MAY_WRITE|NFSD_MAY_LOCAL_ACCESS);
338 if (resp->status && resp->status != nfserr_rofs)
339 goto out_unlock;
340 }
341 } else
342 type = S_IFREG;
343 }
344 } else if (inode) {
345 type = inode->i_mode & S_IFMT;
346 mode = inode->i_mode & ~S_IFMT;
347 } else {
348 type = S_IFREG;
349 mode = 0; /* ??? */
350 }
351
352 attr->ia_valid |= ATTR_MODE;
353 attr->ia_mode = mode;
354
355 /* Special treatment for non-regular files according to the
356 * gospel of sun micro
357 */
358 if (type != S_IFREG) {
359 if (type != S_IFBLK && type != S_IFCHR) {
360 rdev = 0;
361 } else if (type == S_IFCHR && !(attr->ia_valid & ATTR_SIZE)) {
362 /* If you think you've seen the worst, grok this. */
363 type = S_IFIFO;
364 } else {
365 /* Okay, char or block special */
366 if (!rdev)
367 rdev = wanted;
368 }
369
370 /* we've used the SIZE information, so discard it */
371 attr->ia_valid &= ~ATTR_SIZE;
372
373 /* Make sure the type and device matches */
374 resp->status = nfserr_exist;
375 if (inode && inode_wrong_type(inode, type))
376 goto out_unlock;
377 }
378
379 resp->status = nfs_ok;
380 if (!inode) {
381 /* File doesn't exist. Create it and set attrs */
382 resp->status = nfsd_create_locked(rqstp, dirfhp, &attrs, type,
383 rdev, newfhp);
384 } else if (type == S_IFREG) {
385 dprintk("nfsd: existing %s, valid=%x, size=%ld\n",
386 argp->name, attr->ia_valid, (long) attr->ia_size);
387 /* File already exists. We ignore all attributes except
388 * size, so that creat() behaves exactly like
389 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
390 */
391 attr->ia_valid &= ATTR_SIZE;
> 392 if (nfsd_attrs_valid(attr))
393 resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
394 NULL);
395 }
396
397 out_unlock:
398 inode_unlock(dirfhp->fh_dentry->d_inode);
399 fh_drop_write(dirfhp);
400 done:
401 fh_put(dirfhp);
402 if (resp->status != nfs_ok)
403 goto out;
404 resp->status = fh_getattr(&resp->fh, &resp->stat);
405 out:
406 return rpc_success;
407 }
408
On Thu, May 2, 2024 at 6:34 PM Jeffrey Layton <jlayton@kernel.org> wrote: > > On Thu, 2024-05-02 at 15:58 -0400, Stephen Smalley wrote: > > When security labeling is enabled, the client can pass a file security > > label as part of a create operation for the new file, similar to mode > > and other attributes. At present, the security label is received by nfsd > > and passed down to nfsd_create_setattr(), but nfsd_setattr() is never > > called and therefore the label is never set on the new file. I couldn't > > tell if this has always been broken or broke at some point in time. Looking > > at nfsd_setattr() I am uncertain as to whether the same issue presents for > > file ACLs and therefore requires a similar fix for those. I am not overly > > confident that this is the right solution. > > > > An alternative approach would be to introduce a new LSM hook to set the > > "create SID" of the current task prior to the actual file creation, which > > would atomically label the new inode at creation time. This would be better > > for SELinux and a similar approach has been used previously > > (see security_dentry_create_files_as) but perhaps not usable by other LSMs. > > > > Reproducer: > > 1. Install a Linux distro with SELinux - Fedora is easiest > > 2. git clone https://github.com/SELinuxProject/selinux-testsuite > > 3. Install the requisite dependencies per selinux-testsuite/README.md > > 4. Run something like the following script: > > MOUNT=$HOME/selinux-testsuite > > sudo systemctl start nfs-server > > sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT > > sudo mkdir -p /mnt/selinux-testsuite > > sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite > > pushd /mnt/selinux-testsuite/ > > sudo make -C policy load > > pushd tests/filesystem > > sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \ > > -e test_filesystem_filetranscon_t -v > > sudo rm -f trans_test_file > > popd > > sudo make -C policy unload > > popd > > sudo umount /mnt/selinux-testsuite > > sudo exportfs -u localhost:$MOUNT > > sudo rmdir /mnt/selinux-testsuite > > sudo systemctl stop nfs-server > > > > Expected output: > > <eliding noise from commands run prior to or after the test itself> > > Process context: > > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > > Created file: trans_test_file > > File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0 > > File context is correct > > > > Actual output: > > <eliding noise from commands run prior to or after the test itself> > > Process context: > > unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 > > Created file: trans_test_file > > File context: system_u:object_r:test_file_t:s0 > > File context error, expected: > > test_filesystem_filetranscon_t > > got: > > test_file_t > > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > --- > > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by > > Jeffrey Layton <jlayton@kernel.org>. > > > > fs/nfsd/nfsproc.c | 2 +- > > fs/nfsd/vfs.c | 2 +- > > fs/nfsd/vfs.h | 8 ++++++++ > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index 36370b957b63..3e438159f561 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > > * open(..., O_CREAT|O_TRUNC|O_WRONLY). > > */ > > attr->ia_valid &= ATTR_SIZE; > > - if (attr->ia_valid) > > + if (nfsd_attrs_valid(attr)) > > resp->status = nfsd_setattr(rqstp, newfhp, &attrs, > > NULL); > > } > > This function is for NFSv2, which doesn't support any inode attributes > that aren't represented in ia_valid. We could leave this as-is, but > this is fine too. Sorry, I got over-eager with trying to fix all ia_valid checks. It's actually wrong so I'll send a 3rd version without it.
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c index 36370b957b63..3e438159f561 100644 --- a/fs/nfsd/nfsproc.c +++ b/fs/nfsd/nfsproc.c @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) * open(..., O_CREAT|O_TRUNC|O_WRONLY). */ attr->ia_valid &= ATTR_SIZE; - if (attr->ia_valid) + if (nfsd_attrs_valid(attr)) resp->status = nfsd_setattr(rqstp, newfhp, &attrs, NULL); } diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 2e41eb4c3cec..29b1f3613800 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1422,7 +1422,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, * Callers expect new file metadata to be committed even * if the attributes have not changed. */ - if (iap->ia_valid) + if (nfsd_attrs_valid(attrs)) status = nfsd_setattr(rqstp, resfhp, attrs, NULL); else status = nfserrno(commit_metadata(resfhp)); diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index c60fdb6200fd..57cd70062048 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -60,6 +60,14 @@ static inline void nfsd_attrs_free(struct nfsd_attrs *attrs) posix_acl_release(attrs->na_dpacl); } +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs) +{ + struct iattr *iap = attrs->na_iattr; + + return (iap->ia_valid || (attrs->na_seclabel && + attrs->na_seclabel->len)); +} + __be32 nfserrno (int errno); int nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp, struct svc_export **expp);
When security labeling is enabled, the client can pass a file security label as part of a create operation for the new file, similar to mode and other attributes. At present, the security label is received by nfsd and passed down to nfsd_create_setattr(), but nfsd_setattr() is never called and therefore the label is never set on the new file. I couldn't tell if this has always been broken or broke at some point in time. Looking at nfsd_setattr() I am uncertain as to whether the same issue presents for file ACLs and therefore requires a similar fix for those. I am not overly confident that this is the right solution. An alternative approach would be to introduce a new LSM hook to set the "create SID" of the current task prior to the actual file creation, which would atomically label the new inode at creation time. This would be better for SELinux and a similar approach has been used previously (see security_dentry_create_files_as) but perhaps not usable by other LSMs. Reproducer: 1. Install a Linux distro with SELinux - Fedora is easiest 2. git clone https://github.com/SELinuxProject/selinux-testsuite 3. Install the requisite dependencies per selinux-testsuite/README.md 4. Run something like the following script: MOUNT=$HOME/selinux-testsuite sudo systemctl start nfs-server sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT sudo mkdir -p /mnt/selinux-testsuite sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite pushd /mnt/selinux-testsuite/ sudo make -C policy load pushd tests/filesystem sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \ -e test_filesystem_filetranscon_t -v sudo rm -f trans_test_file popd sudo make -C policy unload popd sudo umount /mnt/selinux-testsuite sudo exportfs -u localhost:$MOUNT sudo rmdir /mnt/selinux-testsuite sudo systemctl stop nfs-server Expected output: <eliding noise from commands run prior to or after the test itself> Process context: unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 Created file: trans_test_file File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0 File context is correct Actual output: <eliding noise from commands run prior to or after the test itself> Process context: unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023 Created file: trans_test_file File context: system_u:object_r:test_file_t:s0 File context error, expected: test_filesystem_filetranscon_t got: test_file_t Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by Jeffrey Layton <jlayton@kernel.org>. fs/nfsd/nfsproc.c | 2 +- fs/nfsd/vfs.c | 2 +- fs/nfsd/vfs.h | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-)