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

[Tom Talpey <tom@xxxxxxxxxx>]

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?

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?
- 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".


> >   	__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