Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 May 2023 14:17:14 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Xia Fukun <xiafukun@huawei.com>
Cc:     prajnoha@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6] kobject: Fix global-out-of-bounds in
 kobject_action_type()
Message-ID: <2023051739-shrunk-reopen-b968@gregkh>
References: <20230517101957.14655-1-xiafukun@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230517101957.14655-1-xiafukun@huawei.com>
Precedence: bulk

On Wed, May 17, 2023 at 06:19:57PM +0800, Xia Fukun wrote:
> The following c language code can trigger KASAN's global variable
> out-of-bounds access error in kobject_action_type():
> 
> int main() {
>     int fd;
>     char *filename = "/sys/block/ram12/uevent";
>     char str[86] = "offline";
>     int len = 86;
> 
>     fd = open(filename, O_WRONLY);
>     if (fd == -1) {
>         printf("open");
>         exit(1);
>     }
> 
>     if (write(fd, str, len) == -1) {
>         printf("write");
>         exit(1);
>     }
> 
>     close(fd);
>     return 0;
> }
> 
> Function kobject_action_type() receives the input parameters buf and count,
> where count is the length of the string buf.
> 
> In the use case we provided, count is 86, the count_first is 85.
> Buf points to a string with a length of 86, and its first seven
> characters are "offline".
> In line 87 of the code, kobject_actions[action] is the string "offline"
> with the length of 7,an out-of-boundary access will appear:
> 
> kobject_actions[action][85].
> 
> Use sysfs_match_string() to replace the fragile and convoluted loop.
> This function is well-tested for parsing sysfs inputs. Moreover, this
> modification will not cause any functional changes.
> 
> Fixes: f36776fafbaa ("kobject: support passing in variables for synthetic uevents")
> Signed-off-by: Xia Fukun <xiafukun@huawei.com>
> ---
> v5 -> v6:
> - Ensure that the following extensions remain effective:
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-uevent
> 
> v4 -> v5:
> - Fixed build errors and warnings, and retested the patch.
> 
> v3 -> v4:
> - Refactor the function to be more obviously correct and readable.
> ---
>  include/linux/kobject.h |  3 +++
>  lib/kobject_uevent.c    | 30 +++++++++++++++++-------------
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index c392c811d9ad..9d3ecce3c4f6 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -32,6 +32,9 @@
>  #define UEVENT_NUM_ENVP			64	/* number of env pointers */
>  #define UEVENT_BUFFER_SIZE		2048	/* buffer for the variables */
>  
> +/* the maximum length of the string contained in kobject_actions[] */
> +#define UEVENT_KACT_STRSIZE		16

Why does this value need to be in a global .h file when it is only used
in one .c file?

And how are you going to keep it in sync with kobject_actions if it
changes in the future?  And that variable isn't even in this file, how
would anyone know to modify this if the structure changes in a .c file?

> +
>  #ifdef CONFIG_UEVENT_HELPER
>  /* path to the userspace helper executed on an event */
>  extern char uevent_helper[];
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..4030a928e9c6 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -66,7 +66,8 @@ static int kobject_action_type(const char *buf, size_t count,
>  	enum kobject_action action;
>  	size_t count_first;
>  	const char *args_start;
> -	int ret = -EINVAL;
> +	int i, ret = -EINVAL;
> +	char kobj_act_buf[UEVENT_KACT_STRSIZE] = "";

Why does this need to be initialized?

And are you sure the size is correct?  If so, how?

And how was any of this tested?  Based on your prior submissions, we are
going to require some sort of proof.  What would you do if you were in
my position?

thanks,

greg k-h