Received-SPF: pass (google.com: domain of linux-kernel+bounces-131737-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 4 Apr 2024 18:36:14 +0300
From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Kent Gibson <warthog618@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alexey Dobriyan <adobriyan@gmail.com>, stable@vger.kernel.org,
	Stefan Wahren <wahrenst@gmx.net>,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH v2 2/2] gpio: cdev: fix missed label sanitizing in
 debounce_setup()
Message-ID: <Zg7I7nYkZLcIgETq@smile.fi.intel.com>
References: <20240404093328.21604-1-brgl@bgdev.pl>
 <20240404093328.21604-3-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240404093328.21604-3-brgl@bgdev.pl>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Thu, Apr 04, 2024 at 11:33:28AM +0200, Bartosz Golaszewski wrote:
> From: Kent Gibson <warthog618@gmail.com>
> 
> When adding sanitization of the label, the path through
> edge_detector_setup() that leads to debounce_setup() was overlooked.
> A request taking this path does not allocate a new label and the
> request label is freed twice when the request is released, resulting
> in memory corruption.
> 
> Add label sanitization to debounce_setup().

..

> +static inline char *make_irq_label(const char *orig)
> +{
> +	char *new;
> +
> +	if (!orig)
> +		return NULL;
> +
> +	new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return new;
> +}
> +
> +static inline void free_irq_label(const char *label)
> +{
> +	kfree(label);
> +}

First of all this could have been done in the previous patch already, but okay.

..

> +			label = make_irq_label(line->req->label);
> +			if (IS_ERR(label))
> +				return -ENOMEM;
> +
>  			irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
>  			ret = request_irq(irq, debounce_irq_handler, irqflags,
>  					  line->req->label, line);

But the main point how does this change fix anything?

Shouldn't be

-					  line->req->label, line);
+					  label, line);

?

> +			if (ret) {
> +				free_irq_label(label);
>  				return ret;
> +			}

-- 
With Best Regards,
Andy Shevchenko