Re: [PATCH v4] ksmbd: validate share name from share config response

["Atte Heikkilä" <atteh.mailbox@gmail.com>]

On Sun, 2 Oct 2022 14:34:43 -0400, Tom Talpey wrote:
> On 10/2/2022 11:40 AM, Atte Heikkilä wrote:
>> On Sun,  2 Oct 2022 05:46:28 +0300, Atte Heikkilä wrote:
>>> ...
>>> diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h
>>> index e0cbcfa98c7e..ff07c67f4565 100644
>>> --- a/fs/ksmbd/ksmbd_netlink.h
>>> +++ b/fs/ksmbd/ksmbd_netlink.h
>>> @@ -163,7 +163,8 @@ struct ksmbd_share_config_response {
>>>   	__u16	force_directory_mode;
>>>   	__u16	force_uid;
>>>   	__u16	force_gid;
>>> -	__u32	reserved[128];		/* Reserved room */
>>> +	__s8	share_name[KSMBD_REQ_MAX_SHARE_NAME];
>>> +	__u32	reserved[112];		/* Reserved room */
> 
> I notice you still have "112" here, did you reject my suggestion
> to code the size relative to KSMBD_REQ_MAX_SHARE_NAME?
> 

If size of `reserved' should be relative, then it would be:
128 - DIV_ROUND_UP(KSMBD_REQ_MAX_SHARE_NAME, sizeof(__u32))

Since ksmbd-tools already has the 112 (128-64/4), I thought to keep the
kernel side consistent with it for now.

> Either way I think I made a flawed suggestion. The "reserved" field
> is __u32 but the KSMBD_REQ_MAX_SHARE_NAME is __s8. So, two things:
> 
> - why is share_name an __s8? Wouldn't __u8 be more appropriate?

As I understand it, the only reason for `__s8' over `__u8' here is that
`char' is most often a signed type.

> - why is reserved a __u32? ISTM that __u8 would also be a better
>    choice, and then the size would be "512 - KSMBD_REQ_MAX_SHARE_NAME".
> 
> 

I don't know why `reserved' is `__u32'. I agree that it would be better
if it was `__u8'.

>>>   	__u32	veto_list_sz;
>>>   	__s8	____payload[];
>>>   };
>>> diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c
>>> index 5d039704c23c..dfb4bb365891 100644
>>> --- a/fs/ksmbd/mgmt/share_config.c
>>> +++ b/fs/ksmbd/mgmt/share_config.c
>>> @@ -16,6 +16,7 @@
>>>   #include "user_config.h"
>>>   #include "user_session.h"
>>>   #include "../transport_ipc.h"
>>> +#include "../misc.h"
>>>   
>>>   #define SHARE_HASH_BITS		3
>>>   static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS);
>>> @@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config *share,
>>>   	return 0;
>>>   }
>>>   
>>> -static struct ksmbd_share_config *share_config_request(const char *name)
>>> +static struct ksmbd_share_config *share_config_request(struct unicode_map *um,
>>> +						       const char *name)
>>>   {
>>>   	struct ksmbd_share_config_response *resp;
>>>   	struct ksmbd_share_config *share = NULL;
>>> @@ -133,6 +135,17 @@ static struct ksmbd_share_config *share_config_request(const char *name)
>>>   	if (resp->flags == KSMBD_SHARE_FLAG_INVALID)
>>>   		goto out;
>>>   
>>> +	if (*resp->share_name) {
>>> +		char *cf_resp_name;
>>> +		bool equal;
>>> +
>>> +		cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name);
>>> +		equal = !IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name);
>>> +		kfree(cf_resp_name);
>> 
>> Well, kfree() is *not* a no-op for ERR_PTR() like it is for NULL so
>> this patch is not good either. At least I'm running out of ways to get
>> this wrong.
> 
> :)
> 
> Tom.
> 
> 
>> 
>>> +		if (!equal)
>>> +			goto out;
>>> +	}
>>> +
>>>   	share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL);
>>>   	if (!share)
>>>   		goto out;
>>> @@ -190,7 +203,8 @@ static struct ksmbd_share_config *share_config_request(const char *name)
>>>   	return share;
>>>   }
>>>   
>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name)
>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um,
>>> +						  const char *name)
>>>   {
>>>   	struct ksmbd_share_config *share;
>>>   
>>> @@ -202,7 +216,7 @@ struct ksmbd_share_config *ksmbd_share_config_get(const char *name)
>>>   
>>>   	if (share)
>>>   		return share;
>>> -	return share_config_request(name);
>>> +	return share_config_request(um, name);
>>>   }
>>>   
>>>   bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
>>> diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h
>>> index 7f7e89ecfe61..3fd338293942 100644
>>> --- a/fs/ksmbd/mgmt/share_config.h
>>> +++ b/fs/ksmbd/mgmt/share_config.h
>>> @@ -9,6 +9,7 @@
>>>   #include <linux/workqueue.h>
>>>   #include <linux/hashtable.h>
>>>   #include <linux/path.h>
>>> +#include <linux/unicode.h>
>>>   
>>>   struct ksmbd_share_config {
>>>   	char			*name;
>>> @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct ksmbd_share_config *share)
>>>   	__ksmbd_share_config_put(share);
>>>   }
>>>   
>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name);
>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um,
>>> +						  const char *name);
>>>   bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
>>>   			       const char *filename);
>>>   #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */
>>> diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c
>>> index 867c0286b901..8ce17b3fb8da 100644
>>> --- a/fs/ksmbd/mgmt/tree_connect.c
>>> +++ b/fs/ksmbd/mgmt/tree_connect.c
>>> @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess,
>>>   	struct sockaddr *peer_addr;
>>>   	int ret;
>>>   
>>> -	sc = ksmbd_share_config_get(share_name);
>>> +	sc = ksmbd_share_config_get(conn->um, share_name);
>>>   	if (!sc)
>>>   		return status;
>>>   
>>> @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess,
>>>   		struct ksmbd_share_config *new_sc;
>>>   
>>>   		ksmbd_share_config_del(sc);
>>> -		new_sc = ksmbd_share_config_get(share_name);
>>> +		new_sc = ksmbd_share_config_get(conn->um, share_name);
>>>   		if (!new_sc) {
>>>   			pr_err("Failed to update stale share config\n");
>>>   			status.ret = -ESTALE;
>>> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
>>> index 28459b1efaa8..9e8afaa686e3 100644
>>> --- a/fs/ksmbd/misc.c
>>> +++ b/fs/ksmbd/misc.c
>>> @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path)
>>>   	strreplace(path, '/', '\\');
>>>   }
>>>   
>>> -static char *casefold_sharename(struct unicode_map *um, const char *name)
>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name)
>>>   {
>>>   	char *cf_name;
>>>   	int cf_len;
>>> @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename)
>>>   		name = (pos + 1);
>>>   
>>>   	/* caller has to free the memory */
>>> -	return casefold_sharename(um, name);
>>> +	return ksmbd_casefold_sharename(um, name);
>>>   }
>>>   
>>>   /**
>>> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
>>> index cc72f4e6baf2..1facfcd21200 100644
>>> --- a/fs/ksmbd/misc.h
>>> +++ b/fs/ksmbd/misc.h
>>> @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st);
>>>   void ksmbd_conv_path_to_unix(char *path);
>>>   void ksmbd_strip_last_slash(char *path);
>>>   void ksmbd_conv_path_to_windows(char *path);
>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name);
>>>   char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename);
>>>   char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
>>>   
>>> -- 
>>> 2.37.3
>>>
>>>
>> 
>