On Wed 04-09-19 08:12:50, Steve Magnani wrote:
> Jan -
>
> Tested-by: Steven J. Magnani <steve@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Steven J. Magnani <steve@xxxxxxxxxxxxxxx>
Thanks!
Honza
>
> On 8/29/19 7:25 AM, Jan Kara wrote:
> > OSTA UDF standard defines that domain identifier in logical volume
> > descriptor and file set descriptor should contain a particular string
> > and the identifier suffix contains flags possibly making media
> > write-protected. Verify these constraints and allow only read-only mount
> > if they are not met.
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > fs/udf/ecma_167.h | 14 +++++++++
> > fs/udf/super.c | 91 ++++++++++++++++++++++++++++++++++++++-----------------
> > 2 files changed, 78 insertions(+), 27 deletions(-)
> >
> > diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
> > index 9f24bd1a9f44..fb7f2c7bec9c 100644
> > --- a/fs/udf/ecma_167.h
> > +++ b/fs/udf/ecma_167.h
> > @@ -88,6 +88,20 @@ struct regid {
> > #define ENTITYID_FLAGS_DIRTY 0x00
> > #define ENTITYID_FLAGS_PROTECTED 0x01
> > +/* OSTA UDF 2.1.5.2 */
> > +#define UDF_ID_COMPLIANT "*OSTA UDF Compliant"
> > +
> > +/* OSTA UDF 2.1.5.3 */
> > +struct domainEntityIDSuffix {
> > + uint16_t revision;
> > + uint8_t flags;
> > + uint8_t reserved[5];
> > +};
> > +
> > +/* OSTA UDF 2.1.5.3 */
> > +#define ENTITYIDSUFFIX_FLAGS_HARDWRITEPROTECT 0
> > +#define ENTITYIDSUFFIX_FLAGS_SOFTWRITEPROTECT 1
> > +
> > /* Volume Structure Descriptor (ECMA 167r3 2/9.1) */
> > #define VSD_STD_ID_LEN 5
> > struct volStructDesc {
> > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > index a14346137361..42db3dd51dfc 100644
> > --- a/fs/udf/super.c
> > +++ b/fs/udf/super.c
> > @@ -94,8 +94,8 @@ static int udf_remount_fs(struct super_block *, int *, char *);
> > static void udf_load_logicalvolint(struct super_block *, struct kernel_extent_ad);
> > static int udf_find_fileset(struct super_block *, struct kernel_lb_addr *,
> > struct kernel_lb_addr *);
> > -static void udf_load_fileset(struct super_block *, struct buffer_head *,
> > - struct kernel_lb_addr *);
> > +static int udf_load_fileset(struct super_block *, struct fileSetDesc *,
> > + struct kernel_lb_addr *);
> > static void udf_open_lvid(struct super_block *);
> > static void udf_close_lvid(struct super_block *);
> > static unsigned int udf_count_free(struct super_block *);
> > @@ -757,28 +757,27 @@ static int udf_find_fileset(struct super_block *sb,
> > {
> > struct buffer_head *bh = NULL;
> > uint16_t ident;
> > + int ret;
> > - if (fileset->logicalBlockNum != 0xFFFFFFFF ||
> > - fileset->partitionReferenceNum != 0xFFFF) {
> > - bh = udf_read_ptagged(sb, fileset, 0, &ident);
> > -
> > - if (!bh) {
> > - return 1;
> > - } else if (ident != TAG_IDENT_FSD) {
> > - brelse(bh);
> > - return 1;
> > - }
> > -
> > - udf_debug("Fileset at block=%u, partition=%u\n",
> > - fileset->logicalBlockNum,
> > - fileset->partitionReferenceNum);
> > + if (fileset->logicalBlockNum == 0xFFFFFFFF &&
> > + fileset->partitionReferenceNum == 0xFFFF)
> > + return -EINVAL;
> > - UDF_SB(sb)->s_partition = fileset->partitionReferenceNum;
> > - udf_load_fileset(sb, bh, root);
> > + bh = udf_read_ptagged(sb, fileset, 0, &ident);
> > + if (!bh)
> > + return -EIO;
> > + if (ident != TAG_IDENT_FSD) {
> > brelse(bh);
> > - return 0;
> > + return -EINVAL;
> > }
> > - return 1;
> > +
> > + udf_debug("Fileset at block=%u, partition=%u\n",
> > + fileset->logicalBlockNum, fileset->partitionReferenceNum);
> > +
> > + UDF_SB(sb)->s_partition = fileset->partitionReferenceNum;
> > + ret = udf_load_fileset(sb, (struct fileSetDesc *)bh->b_data, root);
> > + brelse(bh);
> > + return ret;
> > }
> > /*
> > @@ -939,19 +938,53 @@ static int udf_load_metadata_files(struct super_block *sb, int partition,
> > return 0;
> > }
> > -static void udf_load_fileset(struct super_block *sb, struct buffer_head *bh,
> > - struct kernel_lb_addr *root)
> > +static int udf_verify_domain_identifier(struct super_block *sb,
> > + struct regid *ident, char *dname)
>
> The latter two can be 'const'.
>
> > {
> > - struct fileSetDesc *fset;
> > + struct domainEntityIDSuffix *suffix;
> > - fset = (struct fileSetDesc *)bh->b_data;
> > + if (memcmp(ident->ident, UDF_ID_COMPLIANT, strlen(UDF_ID_COMPLIANT))) {
> > + udf_warn(sb, "Not OSTA UDF compliant %s descriptor.\n", dname);
> > + goto force_ro;
> > + }
> > + if (ident->flags & (1 << ENTITYID_FLAGS_DIRTY)) {
> > + udf_warn(sb, "Possibly not OSTA UDF compliant %s descriptor.\n",
> > + dname);
> > + goto force_ro;
> > + }
> > + suffix = (struct domainEntityIDSuffix *)ident->identSuffix;
> > + if (suffix->flags & (1 << ENTITYIDSUFFIX_FLAGS_HARDWRITEPROTECT) ||
> > + suffix->flags & (1 << ENTITYIDSUFFIX_FLAGS_SOFTWRITEPROTECT)) {
> > + if (!sb_rdonly(sb)) {
> > + udf_warn(sb, "Descriptor for %s marked write protected."
> > + " Forcing read only mount.\n", dname);
> > + }
> > + goto force_ro;
> > + }
> > + return 0;
> > - *root = lelb_to_cpu(fset->rootDirectoryICB.extLocation);
> > +force_ro:
> > + if (!sb_rdonly(sb))
> > + return -EACCES;
> > + UDF_SET_FLAG(sb, UDF_FLAG_RW_INCOMPAT);
> > + return 0;
> > +}
> > +static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
> > + struct kernel_lb_addr *root)
> > +{
> > + int ret;
> > +
> > + ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
> > + if (ret < 0)
> > + return ret;
> > +
> > + *root = lelb_to_cpu(fset->rootDirectoryICB.extLocation);
> > UDF_SB(sb)->s_serial_number = le16_to_cpu(fset->descTag.tagSerialNum);
> > udf_debug("Rootdir at block=%u, partition=%u\n",
> > root->logicalBlockNum, root->partitionReferenceNum);
> > + return 0;
> > }
> > int udf_compute_nr_groups(struct super_block *sb, u32 partition)
> > @@ -1364,6 +1397,10 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
> > goto out_bh;
> > }
>
> FYI just a little above this there is a 'BUG_ON(ident != TAG_IDENT_LVD)'
> that would probably be better coded as 'ret = -EINVAL; goto out_bh;'
>
> > + ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
> > + "logical volume");
> > + if (ret)
> > + goto out_bh;
> > ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
> > if (ret)
> > goto out_bh;
> > @@ -2216,9 +2253,9 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
> > UDF_SET_FLAG(sb, UDF_FLAG_RW_INCOMPAT);
> > }
> > - if (udf_find_fileset(sb, &fileset, &rootdir)) {
> > + ret = udf_find_fileset(sb, &fileset, &rootdir);
> > + if (ret < 0) {
>
> Consider making the "No fileset found" warning conditional on ret != -EACCES,
> it's a little confusing to see this in the log:
>
> UDF-fs: warning (device loop0): udf_verify_domain_identifier: Descriptor for file set marked write protected. Forcing read only mount.
> UDF-fs: warning (device loop0): udf_fill_super: No fileset found
>
> > udf_warn(sb, "No fileset found\n");
> > - ret = -EINVAL;
> > goto error_out;
> > }
>
> ------------------------------------------------------------------------
> Steven J. Magnani "I claim this network for MARS!
> www.digidescorp.com Earthling, return my space modulator!"
>
> #include <standard.disclaimer>
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
