[PATCH 1/3] ksmbd: change security id to the one samba used for posix extension

[PATCH 1/3] ksmbd: change security id to the one samba used for posix extension

[Namjae Jeon]

Samba set SIDOWNER and SIDUNIX_GROUP in create posix context and
set SIDUNIX_USER/GROUP in other sids for posix extension.
This patch change security id to the one samba used.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/oplock.c  | 6 +++---
 fs/ksmbd/smb2pdu.c | 4 ++--
 fs/ksmbd/smb2pdu.h | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
index 2e56dac1fa6e..c26f02086783 100644
--- a/fs/ksmbd/oplock.c
+++ b/fs/ksmbd/oplock.c
@@ -1616,7 +1616,7 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
 	memset(buf, 0, sizeof(struct create_posix_rsp));
 	buf->ccontext.DataOffset = cpu_to_le16(offsetof
 			(struct create_posix_rsp, nlink));
-	buf->ccontext.DataLength = cpu_to_le32(52);
+	buf->ccontext.DataLength = cpu_to_le32(56);
 	buf->ccontext.NameOffset = cpu_to_le16(offsetof
 			(struct create_posix_rsp, Name));
 	buf->ccontext.NameLength = cpu_to_le16(POSIX_CTXT_DATA_LEN);
@@ -1642,9 +1642,9 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
 	buf->reparse_tag = cpu_to_le32(fp->volatile_id);
 	buf->mode = cpu_to_le32(inode->i_mode);
 	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
-		  SIDNFS_USER, (struct smb_sid *)&buf->SidBuffer[0]);
+		  SIDOWNER, (struct smb_sid *)&buf->SidBuffer[0]);
 	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
-		  SIDNFS_GROUP, (struct smb_sid *)&buf->SidBuffer[20]);
+		  SIDUNIX_GROUP, (struct smb_sid *)&buf->SidBuffer[28]);
 }
 
 /*
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index f33a04e9e458..bc6c7ce17ea8 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -3573,9 +3573,9 @@ static int smb2_populate_readdir_entry(struct ksmbd_conn *conn, int info_level,
 		if (d_info->hide_dot_file && d_info->name[0] == '.')
 			posix_info->DosAttributes |= FILE_ATTRIBUTE_HIDDEN_LE;
 		id_to_sid(from_kuid_munged(&init_user_ns, ksmbd_kstat->kstat->uid),
-			  SIDNFS_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
+			  SIDUNIX_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
 		id_to_sid(from_kgid_munged(&init_user_ns, ksmbd_kstat->kstat->gid),
-			  SIDNFS_GROUP, (struct smb_sid *)&posix_info->SidBuffer[20]);
+			  SIDUNIX_GROUP, (struct smb_sid *)&posix_info->SidBuffer[16]);
 		memcpy(posix_info->name, conv_name, conv_len);
 		posix_info->name_len = cpu_to_le32(conv_len);
 		posix_info->NextEntryOffset = cpu_to_le32(next_entry_offset);
diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
index af455278d005..32c525bf790a 100644
--- a/fs/ksmbd/smb2pdu.h
+++ b/fs/ksmbd/smb2pdu.h
@@ -158,7 +158,7 @@ struct create_posix_rsp {
 	__le32 nlink;
 	__le32 reparse_tag;
 	__le32 mode;
-	u8 SidBuffer[40];
+	u8 SidBuffer[44];
 } __packed;
 
 struct smb2_buffer_desc_v1 {
@@ -439,7 +439,7 @@ struct smb2_posix_info {
 	__le32 HardLinks;
 	__le32 ReparseTag;
 	__le32 Mode;
-	u8 SidBuffer[40];
+	u8 SidBuffer[32];
 	__le32 name_len;
 	u8 name[1];
 	/*
-- 
2.25.1

[PATCH 2/3] ksmbd: set only file permisson to mode for posix extension

[Namjae Jeon]

Set only file permisson to mode for posix extension like samba.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/oplock.c  | 2 +-
 fs/ksmbd/smb2pdu.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
index c26f02086783..9bfd1ef6debd 100644
--- a/fs/ksmbd/oplock.c
+++ b/fs/ksmbd/oplock.c
@@ -1640,7 +1640,7 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
 
 	buf->nlink = cpu_to_le32(inode->i_nlink);
 	buf->reparse_tag = cpu_to_le32(fp->volatile_id);
-	buf->mode = cpu_to_le32(inode->i_mode);
+	buf->mode = cpu_to_le32(inode->i_mode & 0777);
 	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
 		  SIDOWNER, (struct smb_sid *)&buf->SidBuffer[0]);
 	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index bc6c7ce17ea8..5c797cc09494 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -3565,7 +3565,7 @@ static int smb2_populate_readdir_entry(struct ksmbd_conn *conn, int info_level,
 		posix_info->AllocationSize = cpu_to_le64(ksmbd_kstat->kstat->blocks << 9);
 		posix_info->DeviceId = cpu_to_le32(ksmbd_kstat->kstat->rdev);
 		posix_info->HardLinks = cpu_to_le32(ksmbd_kstat->kstat->nlink);
-		posix_info->Mode = cpu_to_le32(ksmbd_kstat->kstat->mode);
+		posix_info->Mode = cpu_to_le32(ksmbd_kstat->kstat->mode & 0777);
 		posix_info->Inode = cpu_to_le64(ksmbd_kstat->kstat->ino);
 		posix_info->DosAttributes =
 			S_ISDIR(ksmbd_kstat->kstat->mode) ?
@@ -4732,7 +4732,7 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
 	file_info->EndOfFile = cpu_to_le64(inode->i_size);
 	file_info->AllocationSize = cpu_to_le64(inode->i_blocks << 9);
 	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
-	file_info->Mode = cpu_to_le32(inode->i_mode);
+	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
 	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
 	rsp->OutputBufferLength =
 		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
-- 
2.25.1

Re: [PATCH 2/3] ksmbd: set only file permisson to mode for posix extension

[Tom Talpey]

On 9/20/2022 9:20 AM, Namjae Jeon wrote:
> Set only file permisson to mode for posix extension like samba.

Suggest rewording this for clarity:

  "Set file permission mode to match Samba server posix extension behavior".

Acked-by: Tom Talpey <tom@talpey.com>

> 
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/oplock.c  | 2 +-
>   fs/ksmbd/smb2pdu.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
> index c26f02086783..9bfd1ef6debd 100644
> --- a/fs/ksmbd/oplock.c
> +++ b/fs/ksmbd/oplock.c
> @@ -1640,7 +1640,7 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
>   
>   	buf->nlink = cpu_to_le32(inode->i_nlink);
>   	buf->reparse_tag = cpu_to_le32(fp->volatile_id);
> -	buf->mode = cpu_to_le32(inode->i_mode);
> +	buf->mode = cpu_to_le32(inode->i_mode & 0777);
>   	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
>   		  SIDOWNER, (struct smb_sid *)&buf->SidBuffer[0]);
>   	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index bc6c7ce17ea8..5c797cc09494 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -3565,7 +3565,7 @@ static int smb2_populate_readdir_entry(struct ksmbd_conn *conn, int info_level,
>   		posix_info->AllocationSize = cpu_to_le64(ksmbd_kstat->kstat->blocks << 9);
>   		posix_info->DeviceId = cpu_to_le32(ksmbd_kstat->kstat->rdev);
>   		posix_info->HardLinks = cpu_to_le32(ksmbd_kstat->kstat->nlink);
> -		posix_info->Mode = cpu_to_le32(ksmbd_kstat->kstat->mode);
> +		posix_info->Mode = cpu_to_le32(ksmbd_kstat->kstat->mode & 0777);
>   		posix_info->Inode = cpu_to_le64(ksmbd_kstat->kstat->ino);
>   		posix_info->DosAttributes =
>   			S_ISDIR(ksmbd_kstat->kstat->mode) ?
> @@ -4732,7 +4732,7 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
>   	file_info->EndOfFile = cpu_to_le64(inode->i_size);
>   	file_info->AllocationSize = cpu_to_le64(inode->i_blocks << 9);
>   	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
> -	file_info->Mode = cpu_to_le32(inode->i_mode);
> +	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
>   	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
>   	rsp->OutputBufferLength =
>   		cpu_to_le32(sizeof(struct smb311_posix_qinfo));

[PATCH 3/3] ksmbd: fill sids in SMB_FIND_FILE_POSIX_INFO response

[Namjae Jeon]

This patch fill missing sids in SMB_FIND_FILE_POSIX_INFO response.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 5c797cc09494..9dd6033bc4de 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -4717,6 +4717,9 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
 {
 	struct smb311_posix_qinfo *file_info;
 	struct inode *inode = file_inode(fp->filp);
+	struct user_namespace *user_ns = file_mnt_user_ns(fp->filp);
+	vfsuid_t vfsuid = i_uid_into_vfsuid(user_ns, inode);
+	vfsgid_t vfsgid = i_gid_into_vfsgid(user_ns, inode);
 	u64 time;
 
 	file_info = (struct smb311_posix_qinfo *)rsp->Buffer;
@@ -4734,9 +4737,15 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
 	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
 	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
 	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
+
+	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
+		  SIDUNIX_USER, (struct smb_sid *)&file_info->Sids[0]);
+	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
+		  SIDUNIX_GROUP, (struct smb_sid *)&file_info->Sids[16]);
+
 	rsp->OutputBufferLength =
-		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
-	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo));
+		cpu_to_le32(sizeof(struct smb311_posix_qinfo) + 32);
+	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo) + 32);
 	return 0;
 }
 
@@ -4858,7 +4867,7 @@ static int smb2_get_info_file(struct ksmbd_work *work,
 			rc = -EOPNOTSUPP;
 		} else {
 			rc = find_file_posix_info(rsp, fp, work->response_buf);
-			file_infoclass_size = sizeof(struct smb311_posix_qinfo);
+			file_infoclass_size = sizeof(struct smb311_posix_qinfo) + 32;
 		}
 		break;
 	default:
-- 
2.25.1

Re: [PATCH 3/3] ksmbd: fill sids in SMB_FIND_FILE_POSIX_INFO response

[Tom Talpey]

On 9/20/2022 9:20 AM, Namjae Jeon wrote:
> This patch fill missing sids in SMB_FIND_FILE_POSIX_INFO response.
> 
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 5c797cc09494..9dd6033bc4de 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -4717,6 +4717,9 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
>   {
>   	struct smb311_posix_qinfo *file_info;
>   	struct inode *inode = file_inode(fp->filp);
> +	struct user_namespace *user_ns = file_mnt_user_ns(fp->filp);
> +	vfsuid_t vfsuid = i_uid_into_vfsuid(user_ns, inode);
> +	vfsgid_t vfsgid = i_gid_into_vfsgid(user_ns, inode);
>   	u64 time;
>   
>   	file_info = (struct smb311_posix_qinfo *)rsp->Buffer;
> @@ -4734,9 +4737,15 @@ static int find_file_posix_info(struct smb2_query_info_rsp *rsp,
>   	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
>   	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
>   	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
> +
> +	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
> +		  SIDUNIX_USER, (struct smb_sid *)&file_info->Sids[0]);
> +	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
> +		  SIDUNIX_GROUP, (struct smb_sid *)&file_info->Sids[16]);
> +
>   	rsp->OutputBufferLength =
> -		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
> -	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo));
> +		cpu_to_le32(sizeof(struct smb311_posix_qinfo) + 32);
> +	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo) + 32);

These 32's, and the one just below, are really sizeof(sidbuffer), right?

Why code it as a raw number?

Tom.

>   	return 0;
>   }
>   
> @@ -4858,7 +4867,7 @@ static int smb2_get_info_file(struct ksmbd_work *work,
>   			rc = -EOPNOTSUPP;
>   		} else {
>   			rc = find_file_posix_info(rsp, fp, work->response_buf);
> -			file_infoclass_size = sizeof(struct smb311_posix_qinfo);
> +			file_infoclass_size = sizeof(struct smb311_posix_qinfo) + 32;
>   		}
>   		break;
>   	default:

Re: [PATCH 3/3] ksmbd: fill sids in SMB_FIND_FILE_POSIX_INFO response

[Namjae Jeon]

2022-09-21 6:05 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/20/2022 9:20 AM, Namjae Jeon wrote:
>> This patch fill missing sids in SMB_FIND_FILE_POSIX_INFO response.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 5c797cc09494..9dd6033bc4de 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -4717,6 +4717,9 @@ static int find_file_posix_info(struct
>> smb2_query_info_rsp *rsp,
>>   {
>>   	struct smb311_posix_qinfo *file_info;
>>   	struct inode *inode = file_inode(fp->filp);
>> +	struct user_namespace *user_ns = file_mnt_user_ns(fp->filp);
>> +	vfsuid_t vfsuid = i_uid_into_vfsuid(user_ns, inode);
>> +	vfsgid_t vfsgid = i_gid_into_vfsgid(user_ns, inode);
>>   	u64 time;
>>
>>   	file_info = (struct smb311_posix_qinfo *)rsp->Buffer;
>> @@ -4734,9 +4737,15 @@ static int find_file_posix_info(struct
>> smb2_query_info_rsp *rsp,
>>   	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
>>   	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
>>   	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
>> +
>> +	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
>> +		  SIDUNIX_USER, (struct smb_sid *)&file_info->Sids[0]);
>> +	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
>> +		  SIDUNIX_GROUP, (struct smb_sid *)&file_info->Sids[16]);
>> +
>>   	rsp->OutputBufferLength =
>> -		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
>> -	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo));
>> +		cpu_to_le32(sizeof(struct smb311_posix_qinfo) + 32);
>> +	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo) + 32);
>
> These 32's, and the one just below, are really sizeof(sidbuffer), right?
Yes.
>
> Why code it as a raw number?
Sids is declared as flexible-array members.
>
> Tom.
>
>>   	return 0;
>>   }
>>
>> @@ -4858,7 +4867,7 @@ static int smb2_get_info_file(struct ksmbd_work
>> *work,
>>   			rc = -EOPNOTSUPP;
>>   		} else {
>>   			rc = find_file_posix_info(rsp, fp, work->response_buf);
>> -			file_infoclass_size = sizeof(struct smb311_posix_qinfo);
>> +			file_infoclass_size = sizeof(struct smb311_posix_qinfo) + 32;
>>   		}
>>   		break;
>>   	default:
>

Re: [PATCH 3/3] ksmbd: fill sids in SMB_FIND_FILE_POSIX_INFO response

[Tom Talpey]

On 9/20/2022 6:20 PM, Namjae Jeon wrote:
> 2022-09-21 6:05 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/20/2022 9:20 AM, Namjae Jeon wrote:
>>> This patch fill missing sids in SMB_FIND_FILE_POSIX_INFO response.
>>>
>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>> ---
>>>    fs/ksmbd/smb2pdu.c | 15 ++++++++++++---
>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index 5c797cc09494..9dd6033bc4de 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -4717,6 +4717,9 @@ static int find_file_posix_info(struct
>>> smb2_query_info_rsp *rsp,
>>>    {
>>>    	struct smb311_posix_qinfo *file_info;
>>>    	struct inode *inode = file_inode(fp->filp);
>>> +	struct user_namespace *user_ns = file_mnt_user_ns(fp->filp);
>>> +	vfsuid_t vfsuid = i_uid_into_vfsuid(user_ns, inode);
>>> +	vfsgid_t vfsgid = i_gid_into_vfsgid(user_ns, inode);
>>>    	u64 time;
>>>
>>>    	file_info = (struct smb311_posix_qinfo *)rsp->Buffer;
>>> @@ -4734,9 +4737,15 @@ static int find_file_posix_info(struct
>>> smb2_query_info_rsp *rsp,
>>>    	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
>>>    	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
>>>    	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
>>> +
>>> +	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
>>> +		  SIDUNIX_USER, (struct smb_sid *)&file_info->Sids[0]);
>>> +	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
>>> +		  SIDUNIX_GROUP, (struct smb_sid *)&file_info->Sids[16]);
>>> +
>>>    	rsp->OutputBufferLength =
>>> -		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
>>> -	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo));
>>> +		cpu_to_le32(sizeof(struct smb311_posix_qinfo) + 32);
>>> +	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo) + 32);
>>
>> These 32's, and the one just below, are really sizeof(sidbuffer), right?
> Yes.
>>
>> Why code it as a raw number?
> Sids is declared as flexible-array members.

Ugh - worse than that. The smb311_posix_qinfo looks to have
even more undefined payload:

	u8     Sids[];
	/*
	 * var sized owner SID
	 * var sized group SID
	 * le32 filenamelength
	 * u8  filename[]
	 */

This is pre-existing, nothing your patch should address, but
does need attention before we attempt to standardize it!!

MHO anyway.

Tom.


>>
>> Tom.
>>
>>>    	return 0;
>>>    }
>>>
>>> @@ -4858,7 +4867,7 @@ static int smb2_get_info_file(struct ksmbd_work
>>> *work,
>>>    			rc = -EOPNOTSUPP;
>>>    		} else {
>>>    			rc = find_file_posix_info(rsp, fp, work->response_buf);
>>> -			file_infoclass_size = sizeof(struct smb311_posix_qinfo);
>>> +			file_infoclass_size = sizeof(struct smb311_posix_qinfo) + 32;
>>>    		}
>>>    		break;
>>>    	default:
>>
> 

Re: [PATCH 3/3] ksmbd: fill sids in SMB_FIND_FILE_POSIX_INFO response

[Namjae Jeon]

2022-09-23 7:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/20/2022 6:20 PM, Namjae Jeon wrote:
>> 2022-09-21 6:05 GMT+09:00, Tom Talpey <tom@talpey.com>:
>>> On 9/20/2022 9:20 AM, Namjae Jeon wrote:
>>>> This patch fill missing sids in SMB_FIND_FILE_POSIX_INFO response.
>>>>
>>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>>> ---
>>>>    fs/ksmbd/smb2pdu.c | 15 ++++++++++++---
>>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>>> index 5c797cc09494..9dd6033bc4de 100644
>>>> --- a/fs/ksmbd/smb2pdu.c
>>>> +++ b/fs/ksmbd/smb2pdu.c
>>>> @@ -4717,6 +4717,9 @@ static int find_file_posix_info(struct
>>>> smb2_query_info_rsp *rsp,
>>>>    {
>>>>    	struct smb311_posix_qinfo *file_info;
>>>>    	struct inode *inode = file_inode(fp->filp);
>>>> +	struct user_namespace *user_ns = file_mnt_user_ns(fp->filp);
>>>> +	vfsuid_t vfsuid = i_uid_into_vfsuid(user_ns, inode);
>>>> +	vfsgid_t vfsgid = i_gid_into_vfsgid(user_ns, inode);
>>>>    	u64 time;
>>>>
>>>>    	file_info = (struct smb311_posix_qinfo *)rsp->Buffer;
>>>> @@ -4734,9 +4737,15 @@ static int find_file_posix_info(struct
>>>> smb2_query_info_rsp *rsp,
>>>>    	file_info->HardLinks = cpu_to_le32(inode->i_nlink);
>>>>    	file_info->Mode = cpu_to_le32(inode->i_mode & 0777);
>>>>    	file_info->DeviceId = cpu_to_le32(inode->i_rdev);
>>>> +
>>>> +	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
>>>> +		  SIDUNIX_USER, (struct smb_sid *)&file_info->Sids[0]);
>>>> +	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
>>>> +		  SIDUNIX_GROUP, (struct smb_sid *)&file_info->Sids[16]);
>>>> +
>>>>    	rsp->OutputBufferLength =
>>>> -		cpu_to_le32(sizeof(struct smb311_posix_qinfo));
>>>> -	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo));
>>>> +		cpu_to_le32(sizeof(struct smb311_posix_qinfo) + 32);
>>>> +	inc_rfc1001_len(rsp_org, sizeof(struct smb311_posix_qinfo) + 32);
>>>
>>> These 32's, and the one just below, are really sizeof(sidbuffer), right?
>> Yes.
>>>
>>> Why code it as a raw number?
>> Sids is declared as flexible-array members.
>
> Ugh - worse than that. The smb311_posix_qinfo looks to have
> even more undefined payload:
When I checked the behavior of samba, I figured out that samba does
not return filename and filename_length. Only user/group sids... It
seems that the comment is probably wrong, but there is no
specification documentation and there is no way to check it except
packet analysis.
>
> 	u8     Sids[];
> 	/*
> 	 * var sized owner SID
> 	 * var sized group SID
> 	 * le32 filenamelength
> 	 * u8  filename[]
> 	 */
>
> This is pre-existing, nothing your patch should address, but
> does need attention before we attempt to standardize it!!
It can be more updated when standardization is completed, Anyway,
currently I am trying to match the behavior of samba. Please check if
v2 patch is a little better.

Thanks!
>
> MHO anyway.
>
> Tom.
>
>
>>>
>>> Tom.
>>>
>>>>    	return 0;
>>>>    }
>>>>
>>>> @@ -4858,7 +4867,7 @@ static int smb2_get_info_file(struct ksmbd_work
>>>> *work,
>>>>    			rc = -EOPNOTSUPP;
>>>>    		} else {
>>>>    			rc = find_file_posix_info(rsp, fp, work->response_buf);
>>>> -			file_infoclass_size = sizeof(struct smb311_posix_qinfo);
>>>> +			file_infoclass_size = sizeof(struct smb311_posix_qinfo) + 32;
>>>>    		}
>>>>    		break;
>>>>    	default:
>>>
>>
>

Re: [PATCH 1/3] ksmbd: change security id to the one samba used for posix extension

[Tom Talpey]

On 9/20/2022 9:20 AM, Namjae Jeon wrote:
> Samba set SIDOWNER and SIDUNIX_GROUP in create posix context and
> set SIDUNIX_USER/GROUP in other sids for posix extension.
> This patch change security id to the one samba used.
> 
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/oplock.c  | 6 +++---
>   fs/ksmbd/smb2pdu.c | 4 ++--
>   fs/ksmbd/smb2pdu.h | 4 ++--
>   3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
> index 2e56dac1fa6e..c26f02086783 100644
> --- a/fs/ksmbd/oplock.c
> +++ b/fs/ksmbd/oplock.c
> @@ -1616,7 +1616,7 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
>   	memset(buf, 0, sizeof(struct create_posix_rsp));
>   	buf->ccontext.DataOffset = cpu_to_le16(offsetof
>   			(struct create_posix_rsp, nlink));
> -	buf->ccontext.DataLength = cpu_to_le32(52);
> +	buf->ccontext.DataLength = cpu_to_le32(56);

"56" is a weird thing to code here. Can it be expressed as an
offsetof or some sort of sizeof, for clarity and robustness?

>   	buf->ccontext.NameOffset = cpu_to_le16(offsetof
>   			(struct create_posix_rsp, Name));
>   	buf->ccontext.NameLength = cpu_to_le16(POSIX_CTXT_DATA_LEN);
> @@ -1642,9 +1642,9 @@ void create_posix_rsp_buf(char *cc, struct ksmbd_file *fp)
>   	buf->reparse_tag = cpu_to_le32(fp->volatile_id);
>   	buf->mode = cpu_to_le32(inode->i_mode);
>   	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
> -		  SIDNFS_USER, (struct smb_sid *)&buf->SidBuffer[0]);
> +		  SIDOWNER, (struct smb_sid *)&buf->SidBuffer[0]);
>   	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
> -		  SIDNFS_GROUP, (struct smb_sid *)&buf->SidBuffer[20]);
> +		  SIDUNIX_GROUP, (struct smb_sid *)&buf->SidBuffer[28]);

Same comment for "28". offsetof(2 shorts and a sid), right?

>   }
>   
>   /*
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index f33a04e9e458..bc6c7ce17ea8 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -3573,9 +3573,9 @@ static int smb2_populate_readdir_entry(struct ksmbd_conn *conn, int info_level,
>   		if (d_info->hide_dot_file && d_info->name[0] == '.')
>   			posix_info->DosAttributes |= FILE_ATTRIBUTE_HIDDEN_LE;
>   		id_to_sid(from_kuid_munged(&init_user_ns, ksmbd_kstat->kstat->uid),
> -			  SIDNFS_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
> +			  SIDUNIX_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
>   		id_to_sid(from_kgid_munged(&init_user_ns, ksmbd_kstat->kstat->gid),
> -			  SIDNFS_GROUP, (struct smb_sid *)&posix_info->SidBuffer[20]);
> +			  SIDUNIX_GROUP, (struct smb_sid *)&posix_info->SidBuffer[16]);

And for "16", although now I'm also confused why it's 4 *less* than
before.


>   		memcpy(posix_info->name, conv_name, conv_len);
>   		posix_info->name_len = cpu_to_le32(conv_len);
>   		posix_info->NextEntryOffset = cpu_to_le32(next_entry_offset);
> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
> index af455278d005..32c525bf790a 100644
> --- a/fs/ksmbd/smb2pdu.h
> +++ b/fs/ksmbd/smb2pdu.h
> @@ -158,7 +158,7 @@ struct create_posix_rsp {
>   	__le32 nlink;
>   	__le32 reparse_tag;
>   	__le32 mode;
> -	u8 SidBuffer[40];
> +	u8 SidBuffer[44];
>   } __packed;
>   
>   struct smb2_buffer_desc_v1 {
> @@ -439,7 +439,7 @@ struct smb2_posix_info {
>   	__le32 HardLinks;
>   	__le32 ReparseTag;
>   	__le32 Mode;
> -	u8 SidBuffer[40];
> +	u8 SidBuffer[32];

Ok, so it's one buffer, which contains 2 sids? Ick.

>   	__le32 name_len;
>   	u8 name[1];
>   	/*

Re: [PATCH 1/3] ksmbd: change security id to the one samba used for posix extension

[Namjae Jeon]

2022-09-21 5:56 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/20/2022 9:20 AM, Namjae Jeon wrote:
>> Samba set SIDOWNER and SIDUNIX_GROUP in create posix context and
>> set SIDUNIX_USER/GROUP in other sids for posix extension.
>> This patch change security id to the one samba used.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/oplock.c  | 6 +++---
>>   fs/ksmbd/smb2pdu.c | 4 ++--
>>   fs/ksmbd/smb2pdu.h | 4 ++--
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
>> index 2e56dac1fa6e..c26f02086783 100644
>> --- a/fs/ksmbd/oplock.c
>> +++ b/fs/ksmbd/oplock.c
>> @@ -1616,7 +1616,7 @@ void create_posix_rsp_buf(char *cc, struct
>> ksmbd_file *fp)
>>   	memset(buf, 0, sizeof(struct create_posix_rsp));
>>   	buf->ccontext.DataOffset = cpu_to_le16(offsetof
>>   			(struct create_posix_rsp, nlink));
>> -	buf->ccontext.DataLength = cpu_to_le32(52);
>> +	buf->ccontext.DataLength = cpu_to_le32(56);
>
> "56" is a weird thing to code here. Can it be expressed as an
> offsetof or some sort of sizeof, for clarity and robustness?
I'll add a comment above this code.
>
>>   	buf->ccontext.NameOffset = cpu_to_le16(offsetof
>>   			(struct create_posix_rsp, Name));
>>   	buf->ccontext.NameLength = cpu_to_le16(POSIX_CTXT_DATA_LEN);
>> @@ -1642,9 +1642,9 @@ void create_posix_rsp_buf(char *cc, struct
>> ksmbd_file *fp)
>>   	buf->reparse_tag = cpu_to_le32(fp->volatile_id);
>>   	buf->mode = cpu_to_le32(inode->i_mode);
>>   	id_to_sid(from_kuid_munged(&init_user_ns, vfsuid_into_kuid(vfsuid)),
>> -		  SIDNFS_USER, (struct smb_sid *)&buf->SidBuffer[0]);
>> +		  SIDOWNER, (struct smb_sid *)&buf->SidBuffer[0]);
>>   	id_to_sid(from_kgid_munged(&init_user_ns, vfsgid_into_kgid(vfsgid)),
>> -		  SIDNFS_GROUP, (struct smb_sid *)&buf->SidBuffer[20]);
>> +		  SIDUNIX_GROUP, (struct smb_sid *)&buf->SidBuffer[28]);
>
> Same comment for "28". offsetof(2 shorts and a sid), right?
Ditto.
>
>>   }
>>
>>   /*
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index f33a04e9e458..bc6c7ce17ea8 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -3573,9 +3573,9 @@ static int smb2_populate_readdir_entry(struct
>> ksmbd_conn *conn, int info_level,
>>   		if (d_info->hide_dot_file && d_info->name[0] == '.')
>>   			posix_info->DosAttributes |= FILE_ATTRIBUTE_HIDDEN_LE;
>>   		id_to_sid(from_kuid_munged(&init_user_ns, ksmbd_kstat->kstat->uid),
>> -			  SIDNFS_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
>> +			  SIDUNIX_USER, (struct smb_sid *)&posix_info->SidBuffer[0]);
>>   		id_to_sid(from_kgid_munged(&init_user_ns, ksmbd_kstat->kstat->gid),
>> -			  SIDNFS_GROUP, (struct smb_sid *)&posix_info->SidBuffer[20]);
>> +			  SIDUNIX_GROUP, (struct smb_sid *)&posix_info->SidBuffer[16]);
>
> And for "16", although now I'm also confused why it's 4 *less* than
> before.
Ditto.
>
>
>>   		memcpy(posix_info->name, conv_name, conv_len);
>>   		posix_info->name_len = cpu_to_le32(conv_len);
>>   		posix_info->NextEntryOffset = cpu_to_le32(next_entry_offset);
>> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
>> index af455278d005..32c525bf790a 100644
>> --- a/fs/ksmbd/smb2pdu.h
>> +++ b/fs/ksmbd/smb2pdu.h
>> @@ -158,7 +158,7 @@ struct create_posix_rsp {
>>   	__le32 nlink;
>>   	__le32 reparse_tag;
>>   	__le32 mode;
>> -	u8 SidBuffer[40];
>> +	u8 SidBuffer[44];
>>   } __packed;
>>
>>   struct smb2_buffer_desc_v1 {
>> @@ -439,7 +439,7 @@ struct smb2_posix_info {
>>   	__le32 HardLinks;
>>   	__le32 ReparseTag;
>>   	__le32 Mode;
>> -	u8 SidBuffer[40];
>> +	u8 SidBuffer[32];
>
> Ok, so it's one buffer, which contains 2 sids? Ick.
Yes.

Thanks for your review!
>
>>   	__le32 name_len;
>>   	u8 name[1];
>>   	/*
>