| 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
| ||
|
Message-ID: <CACOAw_xEZ+au9yhFerq9amkRO62Dzxj7h71gEc=i16ReYu5xrg@mail.gmail.com>
Date: Wed, 10 Jun 2020 11:05:46 +0900
From: Daeho Jeong <daeho43@...il.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net, kernel-team@...roid.com,
Daeho Jeong <daehojeong@...gle.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl
> > Added a new ioctl to send discard commands or/and zero out
> > to whole data area of a regular file for security reason.
>
> With this ioctl available, what is the exact procedure to write and then later
> securely erase a file on f2fs? In particular, how can the user prevent f2fs
> from making multiple copies of file data blocks as part of garbage collection?
>
To prevent the file data from garbage collecting, the user needs to
use pinfile ioctl and fallocate system call after creating the file.
The sequence is like below.
1. create an empty file
2. pinfile
3. fallocate()
> > +static int f2fs_secure_erase(struct block_device *bdev, block_t block,
> > + block_t len, u32 flags)
> > +{
> > + struct request_queue *q = bdev_get_queue(bdev);
> > + sector_t sector = SECTOR_FROM_BLOCK(block);
> > + sector_t nr_sects = SECTOR_FROM_BLOCK(len);
> > + int ret = 0;
> > +
> > + if (!q)
> > + return -ENXIO;
>
> Why can the request_queue be NULL here?
>
This check is copied from __blkdev_issue_discard(), even if
bdev_get_queue() says it doesn't return NULL value.
__blkdev_issue_discard() does the same thing.
> > +
> > + if (flags & F2FS_TRIM_FILE_DISCARD)
> > + ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS,
> > + blk_queue_secure_erase(q) ?
> > + BLKDEV_DISCARD_SECURE : 0);
> > +
> > + if (!ret && flags & F2FS_TRIM_FILE_ZEROOUT)
> > + ret = blkdev_issue_zeroout(bdev, sector, nr_sects, GFP_NOFS, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> > +{
> > + struct inode *inode = file_inode(filp);
> > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > + struct address_space *mapping = inode->i_mapping;
> > + struct block_device *prev_bdev = NULL;
> > + loff_t file_size;
> > + pgoff_t index, pg_start = 0, pg_end;
> > + block_t prev_block = 0, len = 0;
> > + u32 flags;
> > + int ret = 0;
> > +
> > + if (!S_ISREG(inode->i_mode) || f2fs_is_atomic_file(inode) ||
> > + f2fs_compressed_file(inode))
> > + return -EINVAL;
>
> Is it valid to check f2fs_is_atomic_file() and f2fs_compressed_file() outside of
> inode_lock()?
>
Right, it's better to move the check after inode_lock().
> > +
> > + if (f2fs_readonly(sbi->sb))
> > + return -EROFS;
>
> Isn't this redundant with mnt_want_write_file()?
>
> Also, shouldn't write access to the file be required, i.e.
> (filp->f_mode & FMODE_WRITE)? Then the f2fs_readonly() and
> mnt_want_write_file() checks would be unnecessary.
>
Using FMODE_WRITE is more proper for this case, since we're going to
modify the data. But I think mnt_want_write_file() is still required
to prevent the filesystem from freezing or something else.
> > +
> > + if (f2fs_lfs_mode(sbi))
> > + return -EOPNOTSUPP;
>
> Doesn't this check have to be serialized with remount?
Need to do that, thanks.
>
> > +
> > + if (get_user(flags, (u32 __user *)arg))
> > + return -EFAULT;
> > + if (!(flags & F2FS_TRIM_FILE_MASK))
> > + return -EINVAL;
>
> Need to reject unknown flags:
>
> if (flags & ~F2FS_TRIM_FILE_MASK)
> return -EINVAL;
I needed a different thing here. This was to check neither discard nor
zeroing out are not here. But we still need to check unknown flags,
too.
The below might be better.
if (!flags || flags & ~F2FS_TRIM_FILE_MASK)
return -EINVAL;
>
> > +
> > + if (flags & F2FS_TRIM_FILE_DISCARD && !f2fs_hw_support_discard(sbi))
> > + return -EOPNOTSUPP;
> > +
> > + ret = mnt_want_write_file(filp);
> > + if (ret)
> > + return ret;
> > +
> > + inode_lock(inode);
> > +
> > + file_size = i_size_read(inode);
> > + if (!file_size)
> > + goto err;
>
> ->i_size is stable while holding inode_lock(). So just use ->i_size instead of
> i_size_read().
Yes.
>
> > + pg_end = (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
>
> This can be simplified to:
>
> pg_end = DIV_ROUND_UP(file_size, PAGE_SIZE);
Cool~ :)
>
>
> - Eric
2020년 6월 10일 (수) 오전 1:51, Eric Biggers <ebiggers@...nel.org>님이 작성:
>
> On Tue, Jun 09, 2020 at 03:01:37PM +0900, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@...gle.com>
> >
> > Added a new ioctl to send discard commands or/and zero out
> > to whole data area of a regular file for security reason.
>
> With this ioctl available, what is the exact procedure to write and then later
> securely erase a file on f2fs? In particular, how can the user prevent f2fs
> from making multiple copies of file data blocks as part of garbage collection?
>
> > +static int f2fs_secure_erase(struct block_device *bdev, block_t block,
> > + block_t len, u32 flags)
> > +{
> > + struct request_queue *q = bdev_get_queue(bdev);
> > + sector_t sector = SECTOR_FROM_BLOCK(block);
> > + sector_t nr_sects = SECTOR_FROM_BLOCK(len);
> > + int ret = 0;
> > +
> > + if (!q)
> > + return -ENXIO;
>
> Why can the request_queue be NULL here?
>
> > +
> > + if (flags & F2FS_TRIM_FILE_DISCARD)
> > + ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS,
> > + blk_queue_secure_erase(q) ?
> > + BLKDEV_DISCARD_SECURE : 0);
> > +
> > + if (!ret && flags & F2FS_TRIM_FILE_ZEROOUT)
> > + ret = blkdev_issue_zeroout(bdev, sector, nr_sects, GFP_NOFS, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> > +{
> > + struct inode *inode = file_inode(filp);
> > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > + struct address_space *mapping = inode->i_mapping;
> > + struct block_device *prev_bdev = NULL;
> > + loff_t file_size;
> > + pgoff_t index, pg_start = 0, pg_end;
> > + block_t prev_block = 0, len = 0;
> > + u32 flags;
> > + int ret = 0;
> > +
> > + if (!S_ISREG(inode->i_mode) || f2fs_is_atomic_file(inode) ||
> > + f2fs_compressed_file(inode))
> > + return -EINVAL;
>
> Is it valid to check f2fs_is_atomic_file() and f2fs_compressed_file() outside of
> inode_lock()?
>
> > +
> > + if (f2fs_readonly(sbi->sb))
> > + return -EROFS;
>
> Isn't this redundant with mnt_want_write_file()?
>
> Also, shouldn't write access to the file be required, i.e.
> (filp->f_mode & FMODE_WRITE)? Then the f2fs_readonly() and
> mnt_want_write_file() checks would be unnecessary.
>
> > +
> > + if (f2fs_lfs_mode(sbi))
> > + return -EOPNOTSUPP;
>
> Doesn't this check have to be serialized with remount?
>
> > +
> > + if (get_user(flags, (u32 __user *)arg))
> > + return -EFAULT;
> > + if (!(flags & F2FS_TRIM_FILE_MASK))
> > + return -EINVAL;
>
> Need to reject unknown flags:
>
> if (flags & ~F2FS_TRIM_FILE_MASK)
> return -EINVAL;
>
> > +
> > + if (flags & F2FS_TRIM_FILE_DISCARD && !f2fs_hw_support_discard(sbi))
> > + return -EOPNOTSUPP;
> > +
> > + ret = mnt_want_write_file(filp);
> > + if (ret)
> > + return ret;
> > +
> > + inode_lock(inode);
> > +
> > + file_size = i_size_read(inode);
> > + if (!file_size)
> > + goto err;
>
> ->i_size is stable while holding inode_lock(). So just use ->i_size instead of
> i_size_read().
>
> > + pg_end = (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
>
> This can be simplified to:
>
> pg_end = DIV_ROUND_UP(file_size, PAGE_SIZE);
>
>
> - Eric
Powered by blists - more mailing lists