linux-kernel - Re: [PATCH] fs: 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191019233449.bgimi755vt32itnf@pali>
Date:   Sun, 20 Oct 2019 01:34:49 +0200
From:   Pali Rohár <pali.rohar@...il.com>
To:     Konstantin Komarov <almaz.alexandrovich@...agon-software.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] fs: exFAT read-only driver GPL implementation by Paragon
 Software.

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?

> +	/* 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)?

> +	{ 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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)