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

[Tom Talpey <tom@talpey.com>]

On 10/1/2022 6:53 PM, Atte Heikkilä wrote:
> Share config response may contain the share name without casefolding as
> it is known to the user space daemon. When it is present, casefold and
> compare it to the share name the share config request was made with. If
> they differ, we have a share config which is incompatible with the way
> share config caching is done. This is the case when CONFIG_UNICODE is
> not set, the share name contains non-ASCII characters, and those non-
> ASCII characters do not match those in the share name known to user
> space. In other words, when CONFIG_UNICODE is not set, UTF-8 share
> names now work but are only case-insensitive in the ASCII range.
> 
> Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
> ---
>   v3:
>     - removed initial strcmp() check since it could only save a call to
>       ksmbd_casefold_sharename() for matching ASCII-only share names
> 
>   v2:
>     - no changes were made
> 
>   fs/ksmbd/ksmbd_netlink.h     |  3 ++-
>   fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++---
>   fs/ksmbd/mgmt/share_config.h |  4 +++-
>   fs/ksmbd/mgmt/tree_connect.c |  4 ++--
>   fs/ksmbd/misc.c              |  4 ++--
>   fs/ksmbd/misc.h              |  1 +
>   6 files changed, 27 insertions(+), 9 deletions(-)
> 
> 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 */

Because this is a protocol/abi definition, it may be good practice
to code as
	__u32	reserved[128 - KSMBD_REQ_MAX_SHARE_NAME];
just in case the sharename constant changes in future.

>   	__u32	veto_list_sz;
>   	__s8	____payload[];
>   };
> diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c
> index 5d039704c23c..c62e87b67247 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 = cf_resp_name && !strcmp(cf_resp_name, name);

This is a little bit hard to parse. So, if the casefolding returns
a non-null string, and the result does not equal the original name?
Maybe because it's evaluating a pointer as a bool, and using "!" to
evaluate the strcpy result is the confusing part, to me.

	equal = cf_resp_name != NULL && strcmp(cf_resp_name, name) 0= 0;

Also, strcmp() gets a lot of attention in the kernel these days. Is
this guaranteed safe?

> +		kfree(cf_resp_name);
> +		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)

Another pointer boolean evaluation, but I guess this one is existing.

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

Ditto
>   			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);
>