linux-kernel - Re: [PATCH 2/2] staging: gpib: Agilent usb code cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3a31262b-6bed-4f74-b00e-a124a9486e07@stanley.mountain>
Date: Mon, 20 Jan 2025 10:15:55 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Dave Penkler <dpenkler@...il.com>
Cc: gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] staging: gpib: Agilent usb code cleanup

This patch does too many things...  It should be split up.  People
complain about this requirement a lot, but eventually it will become
instinctive.  I use `git citool` so I can highlight and click to add
lines to a commit.  In this code there were some dev_info() changes
mixed into the unwind code in ->attach() that were hard to separate out
into their own commit but it wasn't too complicated.

On Sat, Jan 18, 2025 at 03:50:46PM +0100, Dave Penkler wrote:
> Remove useless #ifdef RESET_USB_CONFIG code.
> 

patch 1.

> Change kalloc / memset to kzalloc
> 

patch 2.

> The attach function was not freeing the private data on error
> returns. Separate the releasing of urbs and private data and
> add a common error exit for attach failure.
> 
> Set the board private data pointer to NULL after freeing
> the private data.

By setting the private data, this patch actually does fix the
double free that I mentioned earlier.  It changes the ->detach into
a no-op if ->attach fails.  Needs a Fixes tag.  ;)

But I still hope my blog will convince you that the error handling can be
re-written in a better way.  It shouldn't matter if ->private_data is
NULL or non-NULL because the caller should only have to handle success
or failure.  The caller shouldn't have to handle a dozen different
failure modes:

1) Failure but the ->private_data is NULL
2) Failure but the foo->frob pointer is an error pointer
3) Failure but the foo->frob pointer needs to be freed.
4) Failure but the foo->frob pointer contains other pointers which
   need to be freed.
5) ...

It should just be

1) Success: Everything is allocated
2) Failure: Everything is cleaned up and any accesses are probably a
   use after free.

> 
> Reduce console spam by emitting only one attach message.
> 
> Change last pr_err in attach to dev_err
> 

These last two can probably be combined into one patch?

> @@ -1388,11 +1367,19 @@ static int agilent_82357a_attach(gpib_board_t *board, const gpib_board_config_t
>  	retval = agilent_82357a_init(board);
>  
>  	if (retval < 0)	{
> -		mutex_unlock(&agilent_82357a_hotplug_lock);
> -		return retval;
> +		agilent_82357a_cleanup_urbs(a_priv);
> +		agilent_82357a_release_urbs(a_priv);
> +		goto attach_fail;
>  	}

In my blog talk about how every allocation function should have a
matching free() function.  These two functions match
agilent_82357a_setup_urbs() so we should have a single function to
release the urbs.

regards,
dan carpenter