Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 28 Sep 2021 11:55:49 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Yanfei Xu <yanfei.xu@windriver.com>
Cc:     andrew@lunn.ch, hkallweit1@gmail.com, linux@armlinux.org.uk,
        davem@davemloft.net, kuba@kernel.org, p.zabel@pengutronix.de,
        syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register
Message-ID: <20210928085549.GA6559@kili>
References: <20210926045313.2267655-1-yanfei.xu@windriver.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210926045313.2267655-1-yanfei.xu@windriver.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?DpytCGE7ubc7d6OVzyVQgs3qvqcjFYWpySxqzEDc8FK1n2NntRdciuD6IaJt?=
 =?us-ascii?Q?7aJks3DA3fNAEtjSnkV39kSgBejmuMOqft6e38vp4/dixK0MJRLz7ozwEcXi?=
 =?us-ascii?Q?4jn0xJnj+Z5UVcrp47Y3x4aSJU7Xxifht5KSOC4XfRYkFGzKbE2yd+eJ010I?=
 =?us-ascii?Q?hRqFGIFbKV9F+ipj38D7PfarteaAV1EKP9p1xFiPLzn/k6fr4PbXsZ1vVdQB?=
 =?us-ascii?Q?5lUaK624J7InSmqqbYZQ6eV0INh2DmHhI6sQ1lIY2pvZk14WE6Wv/pQQ+fUs?=
 =?us-ascii?Q?N/TEoBLaJtPNPxhhj/BmC7N9bmC6NykTppPxIazzuC2lfu3WQDgp71IUVLYb?=
 =?us-ascii?Q?/DESbAHVbrfMg7y66I2+xCQgzYv/Qbmu4O8c0x3lH93FqIotzC10gXNvyAl9?=
 =?us-ascii?Q?/xtbGhG7TtEiubRTzugO/L5kyaosOB9W7+h6chHlojU6sPNrbtxZ9nE9gBfO?=
 =?us-ascii?Q?Y0ftJh52kko0Z+zqneEXMEov9RENSh9fpTe9kjn0l5fpH1fakgukd46f+jMi?=
 =?us-ascii?Q?4l/VXo+ul+fzfU43/veS4cHscq7iEQ6ZSU+UYFzLR0XTRxbgfU6qQwc/YkF4?=
 =?us-ascii?Q?XtNzDIw585J5chnAUySd19WEwuI5nwBsgpI3hYhVTQ7127z9A257+nvvQVQc?=
 =?us-ascii?Q?X1/DrWPXyewy59Xz+gOb+JS1adnXFIpyUOp4FJf9dv2XdsrNFw0/bCaRjY98?=
 =?us-ascii?Q?RgCxe/ECwastLQ1EP4fLzjzTvGTkPApQqDmlrXWJgcZbMpTmSAgu+bEm1Ida?=
 =?us-ascii?Q?NYCQ3Ro42r7rQ8YSS3ovAsxkMVCkHt17PAa1rjr2HUWe4047+Vs9RBpAHIp1?=
 =?us-ascii?Q?5et56mViGUM8X3kVajrgUoxV+p5m/jY/LthfDqqY4l3kxWe5yWl7EsmKJ1gz?=
 =?us-ascii?Q?/Ks0caIqu67vNlnl1bpQzc5VCjw0sneILcc9qvx4W4iAWemTDxguAX5Dyk0F?=
 =?us-ascii?Q?Ce9c0TY8CAecBSIkhc6x28uHf2EIaEatfCd+dP1zzmG1q7zfAMTO2WAYWcED?=
 =?us-ascii?Q?hnH1rnBSUn2DUlZcnXZM0fB88idZeqhF0eK7uF9PTIMEeLGpfo2b41tsjmFG?=
 =?us-ascii?Q?vWYH+EUo3XVppaZCi06xa+jv4HLAUUw/vyUj/e7Qnkxqb5/oI84COeAAoqUN?=
 =?us-ascii?Q?5Ho6l39bgCevKutm22IXCeb5JQ8YO38o5mC7a6SbpOOK1T3gWuILuZAHbNjm?=
 =?us-ascii?Q?2PYEp1AsqE2e/YCQyfS5oaUHEahn1yeTfboCksKaGq+4yRY4ZieQ20wu/mGX?=
 =?us-ascii?Q?DdgUco2dH6SyTqHyy4TQwjz9aVMs9+1hpaRuWuXL8ap0pekJ3RcPaKoWN8mR?=
 =?us-ascii?Q?Lg2oxqZjp46/8X1C9QoU0Gff?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 71a1e9c4-d75a-4477-b169-08d9825dcdb6
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2021 08:56:04.8755
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZkQsDZtMNYYHVDIVhYth59kr2TM+XR1GTNQVpKZC7DbMN6bb8RQZlL1aPcRXv7qINWzfAZ50TufvL6GjtfAEGFFlN79fGWlZrtQHqq85RW0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1421
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10120 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 adultscore=0
 mlxscore=0 malwarescore=0 suspectscore=0 bulkscore=0 mlxlogscore=775
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2109280052
X-Proofpoint-ORIG-GUID: wCiU8NugDGwYB_S-kEMp05EBV4sV6kiS
X-Proofpoint-GUID: wCiU8NugDGwYB_S-kEMp05EBV4sV6kiS
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu wrote:
> Once device_register() failed, we should call put_device() to
> decrement reference count for cleanup. Or it will cause memory
> leak.
> 

[ snipped stack trace ]

> 
> Reported-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>
> ---
>  drivers/net/phy/mdio_bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 53f034fc2ef7..6f4b4e5df639 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>  	err = device_register(&bus->dev);
>  	if (err) {
>  		pr_err("mii_bus %s failed to register\n", bus->id);
> +		put_device(&bus->dev);

No this isn't right.  The dev.class is &mdio_bus_class.  It's set a few
lines earlier.

	bus->dev.class = &mdio_bus_class;

The release function is mdiobus_release().  It will free bus and lead to
use after frees in the callers.  Look at greth_mdio_init().  There are
a lot of callers which will crash now.

This patch was a clear layering violation.  If you didn't allocate "bus"
then you should not free it.  Keeping that rule in mind can help prevent
future bugs.  Also he other error handling paths are careful not to call
put_device() so why would this one be special?  It should have stood out
that this one error path is the only place that doesn't use a goto to
clean up.

I don't have a solution.  I have commented before that I hate kobjects
for this reason because they lead to unfixable memory leaks during
probe.  But this leak will only happen with fault injection and so it
doesn't affect real life.  And even if it did, a leak it preferable to a
crash.

Unfortunately, this patch has already been applied so please can you
send a patch to revert it?

regards,
dan carpenter