linux-kernel - Re: [PATCH] exFAT read-only driver GPL implementation by Paragon Software.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9763488C-9431-49FF-9465-1B965F159120@paragon-software.com>
Date:   Tue, 22 Oct 2019 17:13:44 +0000
From:   Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
To:     Pali Rohár <pali.rohar@...il.com>
CC:     "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] exFAT read-only driver GPL implementation by Paragon
 Software.

Hello! 
Thanks for your findings.

> On 20 Oct 2019, at 02:34, Pali Rohár <pali.rohar@...il.com> wrote:
> 
> Hello! I have not read deeply whole implementation, just spotted
> suspicious options. See below.
> 
> On Friday 18 October 2019 15:18:39 Konstantin Komarov wrote:
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> new file mode 100644
>> index 000000000000..5f8713fe1b0c
>> --- /dev/null
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -0,0 +1,388 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  linux/fs/exfat/super.c
>> + *
>> + * Copyright (c) 2010-2019 Paragon Software GmbH, All rights reserved.
>> + *
>> + */
>> +
>> +#include <linux/buffer_head.h>
>> +#include <linux/hash.h>
>> +#include <linux/nls.h>
>> +#include <linux/ratelimit.h>
>> +
>> +struct exfat_mount_options {
>> +	kuid_t fs_uid;
>> +	kgid_t fs_gid;
>> +	u16 fs_fmask;
>> +	u16 fs_dmask;
>> +	u16 codepage; /* Codepage for shortname conversions */
> 
> According to exFAT specification, section 7.7.3 FileName Field there is
> no 8.3 shortname support with DOS/OEM codepage.
> 
> https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#773-filename-field
> 
> Plus it looks like that this member codepage is only set and never
> accessed in whole driver.
> 
> So it can be clean it up and removed?
Yes, this one should be removed as not used actually through the code. One thing to do about the implementation, besides RW support and a little clean-up of such places like this one, is nls/codepage support. Currently, utf-8 only is supported.

> 
>> +	/* minutes bias= UTC - local time. Eastern time zone: +300, */
>> +	/*Paris,Berlin: -60, Moscow: -180*/
>> +	int bias;
>> +	u16 allow_utime; /* permission for setting the [am]time */
>> +	unsigned quiet : 1, /* set = fake successful chmods and chowns */
>> +		showexec : 1, /* set = only set x bit for com/exe/bat */
>> +		sys_immutable : 1, /* set = system files are immutable */
>> +		utf8 : 1, /* Use of UTF-8 character set (Default) */
>> +		/* create escape sequences for unhandled Unicode */
>> +		unicode_xlate : 1, flush : 1, /* write things quickly */
>> +		tz_set : 1, /* Filesystem timestamps' offset set */
>> +		discard : 1 /* Issue discard requests on deletions */
>> +		;
>> +};
> 
> ...
> 
>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>> new file mode 100644
>> index 000000000000..0705dab3c3fc
>> --- /dev/null
>> +++ b/fs/exfat/super.c
> ...
>> +enum {
>> +	Opt_uid, Opt_gid, Opt_umask, Opt_dmask, Opt_fmask, Opt_allow_utime,
>> +	Opt_codepage, Opt_quiet, Opt_showexec, Opt_debug, Opt_immutable,
>> +	Opt_utf8_no, Opt_utf8_yes, Opt_uni_xl_no, Opt_uni_xl_yes, Opt_flush,
>> +	Opt_tz_utc, Opt_discard, Opt_nfs, Opt_bias, Opt_err,
>> +};
>> +
>> +static const match_table_t fat_tokens = {
>> +	{ Opt_uid, "uid=%u" },
>> +	{ Opt_gid, "gid=%u" },
>> +	{ Opt_umask, "umask=%o" },
>> +	{ Opt_dmask, "dmask=%o" },
>> +	{ Opt_fmask, "fmask=%o" },
>> +	{ Opt_allow_utime, "allow_utime=%o" },
>> +	{ Opt_codepage, "codepage=%u" },
>> +	{ Opt_quiet, "quiet" },
>> +	{ Opt_showexec, "showexec" },
>> +	{ Opt_debug, "debug" },
>> +	{ Opt_immutable, "sys_immutable" },
>> +	{ Opt_flush, "flush" },
>> +	{ Opt_tz_utc, "tz=UTC" },
>> +	{ Opt_bias, "bias=%d" },
>> +	{ Opt_discard, "discard" },
>> +	{ Opt_utf8_no, "utf8=0" }, /* 0 or no or false */
>> +	{ Opt_utf8_no, "utf8=no" },
>> +	{ Opt_utf8_no, "utf8=false" },
>> +	{ Opt_utf8_yes, "utf8=1" }, /* empty or 1 or yes or true */
>> +	{ Opt_utf8_yes, "utf8=yes" },
>> +	{ Opt_utf8_yes, "utf8=true" },
>> +	{ Opt_utf8_yes, "utf8" },
> 
> There are lot of utf8 mount options. Are they really needed?
> 
> Would not it be better to use just one "iocharset" mount option like
> other Unicode based filesystem have it (e.g. vfat, jfs, iso9660, udf or
> ntfs)?

It seems reasonable as well. "iocharset" may be more handy way to handle such mount options.

> 
>> +	{ Opt_uni_xl_no, "uni_xlate=0" }, /* 0 or no or false */
>> +	{ Opt_uni_xl_no, "uni_xlate=no" },
>> +	{ Opt_uni_xl_no, "uni_xlate=false" },
>> +	{ Opt_uni_xl_yes, "uni_xlate=1" }, /* empty or 1 or yes or true */
>> +	{ Opt_uni_xl_yes, "uni_xlate=yes" },
>> +	{ Opt_uni_xl_yes, "uni_xlate=true" },
>> +	{ Opt_uni_xl_yes, "uni_xlate" },
>> +	{ Opt_err, NULL }
>> +};
> 
> -- 
> Pali Rohár
> pali.rohar@...il.com