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

[Tom Talpey <tom@xxxxxxxxxx>]

On 10/2/2022 10:06 PM, Namjae Jeon wrote:
> 2022-10-03 6:21 GMT+09:00, Atte Heikkilä <atteh.mailbox@xxxxxxxxx>:
> > 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'.
> We can make another patch for both ksmbd/ksmbd-tools later.

Two copies of this whole structure is a bug waiting to happen.

The best approach would be to place the definition in a single shared
header file. And it may need some sort of version stamp, if that huge
"reserved" block is to be used.

I believe the kernel include/uapi tree is an established place for this
type of user/kernel interface definition. A side benefit is that because
of the ABI implications, changes there are pretty well-vetted.

Tom.

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