linux-kernel - Re: [PATCH v5 5/8] fat: restructure export

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: <CAKYAXd_T2yGRXd3bk5HXXuVhx_ikO1xzinOy0d8TobokyiCRsA@mail.gmail.com>
Date:	Wed, 5 Dec 2012 20:45:40 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc:	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Namjae Jeon <namjae.jeon@...sung.com>,
	Ravishankar N <ravi.n1@...sung.com>,
	Amit Sahrawat <a.sahrawat@...sung.com>
Subject: Re: [PATCH v5 5/8] fat: restructure export_operations

2012/12/5, OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>:
> Namjae Jeon <linkinjeon@...il.com> writes:
>
>>>> Let me think, if ‘subtree’ checking is enabled then we should check
>>>> the length condition over here also? Please share if there are any
>>>> other comments also.
>>>
>>> I'm not sure what did you mean. Where is "subtree" check you are
>>> talking? This is fh_to_dentry(), so we don't use parent at all, so
>>> length == 3 is enough?
>> With fileID type="FILEID_FAT_WITHOUT_PARENT", fhlen  should be '3'
>> With fileId type="FILEID_FAT_WITH_PARENT", fhlen should be '5'
>>
>> While encoding, WITH_PARENT is selected when subtree check is enabled
>> on the NFS Server.
>>
>> So, when decoding request is arrived-  fileid type will be among the '2'
>> cases:
>> Now, in case of fh_to_dentry() - when we consider, that the reqquest
>> for fileid type WITH_PARENT
>> then i think the conditions in fh_to_dentry should be:
>>
>> if((fh_type == FILEID_FAT_WITH_PARENT) && fh_len != 5)
>> 	return NULL;
>> else if (fh_len != 3)
>> 	return NULL;
>>
>> So, we took care of these '2' conditions within the switch statement
>> based on the 'fh_type'. We can just change the comparision condition
>> from '<' to '!=':
>> switch (fh_type) {
>> +	case FILEID_FAT_WITHOUT_PARENT:
>> +		if (fh_len != FAT_FID_SIZE_WITHOUT_PARENT)
>> +			return NULL;
>> +	case FILEID_FAT_WITH_PARENT:
>> +		if ((fh_len != FAT_FID_SIZE_WITH_PARENT) &&
>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>> +			return NULL;
>> +		i_pos = fid->i_pos_hi;
>> +		i_pos = (i_pos << 32) | (fid->i_pos_low);
>> +		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
>> +		break;
>> +	}
>>
>> I think there is no need to push the comparision statements in the
>> begining similar to generic_fh_to_dentry.
>
> I can understand what is doing. I'm asking why there is difference.
>
> 1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
> 2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).
>
> Why does logic has difference?

When we consider the generic routine of encode_fh() and the structure ‘fid’

struct fid {
      struct {
            u32 ino;
            u32 gen;
            u32 parent_ino;
            u32 parent_gen;
      } i32;
};

fh_len= 2(without parent)
fh_len=4(with parent)

Condition checking in export_encode_fh()
{

   if (parent && (len < 4)) {
                *max_len = 4;
                return FILEID_INVALID;
        } else if (len < 2) {
                *max_len = 2;
                return FILEID_INVALID;
        }
        ...
                len = 2;
        ...
                if (parent) {
..
                                len = 4;
                                type = FILEID_INO32_GEN_PARENT;
                }
…
}

The logic does take care of altering the length for the ‘2’ cases
with/without parent.
So, while encoding -> the care has been taken for length checking but
while decoding(generic_fh_to_dentry) the length check is not put in
place.
I think it should be done in the generic routine also.

It should be:
if ((fh_len != 2 && fh_len != 4) ||
                (fh_type != FILEID_INO32_GEN && fh_type !=
FILEID_INO32_GEN_PARENT))
    return NULL;

Please share your opinion.

Thanks.
> --
> OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/