Re: [PATCH] liveupdate: validate session type before performing operation

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On 05-19 14:24, Pratyush Yadav wrote:
> From: "Pratyush Yadav (Google)" <pratyush@kernel.org>
> 
> The sessions ioctls are not applicable to all session types. PRESERVE_FD
> is only applicable to outgoing sessions. RETRIEVE_FD and FINISH are only
> valid for incoming session. Calling a incoming ioctl on an outgoing
> session is invalid and can cause file handlers to run into unexpected
> errors.
> 
> For example, a user can create a (outgoing) session, preserve a memfd,
> and then immediately do a retrieve without doing a kexec in between.

Please add a self-test tools/testing/selftests/liveupdate/liveupdate.c
to verify that outgoing sessions do not accept retrieve_fd ioctl.
Option, you could also add to luo_multi_session.c a test to verifying 
that incoming does not accept preserve_fd

> This would result in memfd's retrieve handler to run. The handlers
> expects to be called from a post-kexec context, and will try to do a
> kho_restore_vmalloc() or kho_restore_folio() to try and restore memory.
> 
> KHO catches this (thanks to KHO_PAGE_MAGIC) and returns an error, but
> since this is considered an internal error and KHO throws out a bunch of
> WARN()s.
> 
> Associate a type with each ioctl op and validate the type in
> luo_session_ioctl() before dispatching the ioctl handler to make sure
> the op is being called for the right session type.
> 
> Fixes: 16cec0d26521 ("liveupdate: luo_session: add ioctls for file preservation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pratyush Yadav (Google) <pratyush@kernel.org>
> ---
> 
> Notes:
>     I added LUO_IOCTL_ALL but there is no user in this patch. The type is
>     for LIVEUPDATE_SESSION_GET_NAME which is supported for both session
>     types. The support for GET_NAME is in next and this patch should go
>     through fixes.
>     
>     Alternatively, we can remove LUO_IOCTL_ALL from this patch and add it to
>     the LIVEUPDATE_SESSION_GET_NAME patch in next. But that would need us to
>     rebase to an rc that contains this fix.

Let's keep LUO_IOCTL_ALL change in this patch.

Please add tests, otherwise LGTM.

Pasha

> 
>  kernel/liveupdate/luo_session.c | 36 +++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> index a3327a28fc1f..e84218e3cacb 100644
> --- a/kernel/liveupdate/luo_session.c
> +++ b/kernel/liveupdate/luo_session.c
> @@ -295,32 +295,58 @@ union ucmd_buffer {
>  	struct liveupdate_session_retrieve_fd retrieve;
>  };
>  
> +/* Type of sessions the ioctl applies to. */
> +enum luo_ioctl_type {
> +	LUO_IOCTL_INCOMING,
> +	LUO_IOCTL_OUTGOING,
> +	LUO_IOCTL_ALL,
> +};
> +
>  struct luo_ioctl_op {
>  	unsigned int size;
>  	unsigned int min_size;
>  	unsigned int ioctl_num;
> +	enum luo_ioctl_type type;
>  	int (*execute)(struct luo_session *session, struct luo_ucmd *ucmd);
>  };
>  
> -#define IOCTL_OP(_ioctl, _fn, _struct, _last)                                  \
> +#define IOCTL_OP(_ioctl, _fn, _struct, _last, _type)                           \
>  	[_IOC_NR(_ioctl) - LIVEUPDATE_CMD_SESSION_BASE] = {                    \
>  		.size = sizeof(_struct) +                                      \
>  			BUILD_BUG_ON_ZERO(sizeof(union ucmd_buffer) <          \
>  					  sizeof(_struct)),                    \
>  		.min_size = offsetofend(_struct, _last),                       \
>  		.ioctl_num = _ioctl,                                           \
> +		.type = _type,                                                 \
>  		.execute = _fn,                                                \
>  	}
>  
>  static const struct luo_ioctl_op luo_session_ioctl_ops[] = {
>  	IOCTL_OP(LIVEUPDATE_SESSION_FINISH, luo_session_finish,
> -		 struct liveupdate_session_finish, reserved),
> +		 struct liveupdate_session_finish, reserved, LUO_IOCTL_INCOMING),
>  	IOCTL_OP(LIVEUPDATE_SESSION_PRESERVE_FD, luo_session_preserve_fd,
> -		 struct liveupdate_session_preserve_fd, token),
> +		 struct liveupdate_session_preserve_fd, token, LUO_IOCTL_OUTGOING),
>  	IOCTL_OP(LIVEUPDATE_SESSION_RETRIEVE_FD, luo_session_retrieve_fd,
> -		 struct liveupdate_session_retrieve_fd, token),
> +		 struct liveupdate_session_retrieve_fd, token, LUO_IOCTL_INCOMING),
>  };
>  
> +static bool luo_ioctl_type_valid(struct luo_session *session,
> +				 const struct luo_ioctl_op *op)
> +{
> +	switch (op->type) {
> +	case LUO_IOCTL_INCOMING:
> +		/* Retrieved is only set on incoming sessions */
> +		return session->retrieved;
> +	case LUO_IOCTL_OUTGOING:
> +		return !session->retrieved;
> +	case LUO_IOCTL_ALL:
> +		return true;
> +	}
> +
> +	/* Catch-all. */
> +	return false;
> +}
> +
>  static long luo_session_ioctl(struct file *filep, unsigned int cmd,
>  			      unsigned long arg)
>  {
> @@ -345,6 +371,8 @@ static long luo_session_ioctl(struct file *filep, unsigned int cmd,
>  	op = &luo_session_ioctl_ops[nr - LIVEUPDATE_CMD_SESSION_BASE];
>  	if (op->ioctl_num != cmd)
>  		return -ENOIOCTLCMD;
> +	if (!luo_ioctl_type_valid(session, op))
> +		return -EINVAL;
>  	if (ucmd.user_size < op->min_size)
>  		return -EINVAL;
>  
> 
> base-commit: b1378127003b61930ce30064328640503ad3ef6d
> -- 
> 2.54.0.563.g4f69b47b94-goog
>