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:   Fri, 28 Oct 2022 20:06:34 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Sowjanya Komatineni <skomatineni@nvidia.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        stable@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        linux-media@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: media: tegra-video: fix device_node use after
 free
Message-ID: <Y1wMGpthKxr2egtY@kadam>
References: <20221028081926.2320663-1-luca.ceresoli@bootlin.com>
 <Y1vMX/Zciz/XQ+4p@kadam>
 <20221028185847.5454a98d@booty>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221028185847.5454a98d@booty>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OYE4NbqAzUzWhl/lp1iB44IE3ciT8VlfaK6ayuGNH49HKmZhUQlPqhIxUfjB?=
 =?us-ascii?Q?h3fkv3Per/NUR1p9dhOPN1Zm00RsTYjQd3MJeIqPRkfwmyufgRCmk/BkZv94?=
 =?us-ascii?Q?5dz91yETgXqYUbneHy5WYDPu3qdPC4kpTqdW407Cl3WN4Z2lZ13bGp4eqW6f?=
 =?us-ascii?Q?PXNbWlZOqg3/59yu4bOgbNVBuQZXv7ZUQov35SppSWLY1ZvcaIAhd1jI9M4y?=
 =?us-ascii?Q?+9/Sw+456C8HjWbo5Zk95jWdQEFwyCgfrMUdTEWnVJS9Rjed9+mQvMU0JvWe?=
 =?us-ascii?Q?y+5s2OaqBfUMxPOGL9BxBO/zCpwWeQ/zuXfXgAsHxik8GnbZodRAl0n5lEpq?=
 =?us-ascii?Q?gGRA5jmLnLVNVN8jOYw4fEL0fKWqpz8XBBor3Een9qFMkAOOkf62e5UvwTNa?=
 =?us-ascii?Q?cszewxTz98tBSrycOz6MpqbwSznZHfFZdNI/ymxzCQVNt8iUoqhTYfHnuZUZ?=
 =?us-ascii?Q?E/QGGAOV4CzZmEm0AR7QUdeme6YifXn8ReKIIRmAqn4VVSCj5jn2Savmwy79?=
 =?us-ascii?Q?GR44f80ivZ2XAaPbJhM9K3hrybymCQCfIu3EbkZLIf8yn19Qjw/vxOjSSsvv?=
 =?us-ascii?Q?jQzlfOxka3+L5lw/rfIuQ9dbXm1Hv+0VJlShjCeCWM77nD38erj83UX5Wu+5?=
 =?us-ascii?Q?88E616c0TNcIbZEuhdqg1SXn17N1fBALE2BKuDMxBODzdnLg6hXsw+JZHoql?=
 =?us-ascii?Q?0hQlO8Rzz2rc3eSst1gtU5TltLmpsM+jFZGWzdCVwQOyrA6XoDUP8jU0NZXZ?=
 =?us-ascii?Q?zP4uHhDY6vBa5XzHZtPDV0AkloEXVzPU+r/FXtCwdACMuskwDILuyFdZtSfZ?=
 =?us-ascii?Q?TyTvRUGE+dkJx/vybW3YQcoD3+SV0lASUUf5Fxq/u4BFyuREIEuuTFi42pRI?=
 =?us-ascii?Q?iUYNqtitovYLVAushYCi0eM/i25RYyeBVmO+cLBeFJD+9icVPcLe7z0o1XAn?=
 =?us-ascii?Q?eJzTb1u9DKd35RCv7HewW2O7rwicR+D3zPBH+zm5XGApXezRhidduESxdK8W?=
 =?us-ascii?Q?OI9FrO1ZshIa2vCFk63f15TLak0T+qb98wwnC1y4dEe/E01ZtTbSpwyEylnt?=
 =?us-ascii?Q?GpCkd6dVnTOVsQtQdFIA8UluE0ik/7HgEBQV+IcE70amEMLxdQZ/npcJuL9y?=
 =?us-ascii?Q?rddUStfK5m44GQnIXb6zRg1PsZ+WjH4uepLNEwGp2xyr9AQFzHuUgkOCM9R2?=
 =?us-ascii?Q?LT/746GcR+i5J/+JadhEwzzdkpqU4CjmrPHr65FGvBwj1x6GDgypj/IHevob?=
 =?us-ascii?Q?TgT1vcc7KjpXaohZQpa2MtZmxalrb6CcE85izw7h77H1CwL5+PX8ttU1p5J6?=
 =?us-ascii?Q?oyjD61xJILVnPfH5rlFW4lZ17swgqQvipy3IypSigZYA3ocIByv4Im5SW4nX?=
 =?us-ascii?Q?XWoKVUS8a00DXbKAvUIr/eO1bLR5vc8vr/jIAs5fuhDTnbXwc2Cr2WUAPPkS?=
 =?us-ascii?Q?JHe6b00WBHW0MJWmsnJt7CV05AT9TeCOcxdR0tfF//3I2gjb6aIg4z14gylm?=
 =?us-ascii?Q?O8J5kju0ll0htgzEk1mts3qWmhr+MiSssYWqO6o1Dt6tW9nmPJvYwdnuoozq?=
 =?us-ascii?Q?acStNJXH8zrFIgg7Ljyghf5i5UQw339vhO5wwYd81OFj/R1OtnYXQwVP2lj2?=
 =?us-ascii?Q?AQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2152b606-b177-4aee-e377-08dab906caf6
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2022 17:06:45.4497
 (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: gJCclLxBSCDSXAx6sxnAf0g5hWtGVwSFOdmG1PBwKKJy84pY4E0iuMS5roDtvjLQs0BIghu6FC+fkpy2oWp6D+/hmV68VZcDeew+NX6Jgb4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6445
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-28_07,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=982
 suspectscore=0 mlxscore=0 spamscore=0 phishscore=0 malwarescore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210280107
X-Proofpoint-GUID: uDr0FdCpIu5e2OsenvSGAze26p4Nv9vr
X-Proofpoint-ORIG-GUID: uDr0FdCpIu5e2OsenvSGAze26p4Nv9vr
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Oct 28, 2022 at 06:58:47PM +0200, Luca Ceresoli wrote:
> Hello Dan,
> 
> On Fri, 28 Oct 2022 15:34:39 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Fri, Oct 28, 2022 at 10:19:26AM +0200, luca.ceresoli@bootlin.com wrote:
> > > From: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > 
> > > At probe time this code path is followed:
> > > 
> > >  * tegra_csi_init
> > >    * tegra_csi_channels_alloc
> > >      * for_each_child_of_node(node, channel) -- iterates over channels
> > >        * automatically gets 'channel'
> > >          * tegra_csi_channel_alloc()
> > >            * saves into chan->of_node a pointer to the channel OF node
> > >        * automatically gets and puts 'channel'
> > >        * now the node saved in chan->of_node has refcount 0, can disappear
> > >    * tegra_csi_channels_init
> > >      * iterates over channels
> > >        * tegra_csi_channel_init -- uses chan->of_node
> > > 
> > > After that, chan->of_node keeps storing the node until the device is
> > > removed.
> > > 
> > > of_node_get() the node and of_node_put() it during teardown to avoid any
> > > risk.
> > > 
> > > Fixes: 1ebaeb09830f ("media: tegra-video: Add support for external sensor capture")
> > > Cc: stable@vger.kernel.org
> > > Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > ---
> > >  drivers/staging/media/tegra-video/csi.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> > > index b26e44adb2be..1b05f620b476 100644
> > > --- a/drivers/staging/media/tegra-video/csi.c
> > > +++ b/drivers/staging/media/tegra-video/csi.c
> > > @@ -433,7 +433,7 @@ static int tegra_csi_channel_alloc(struct tegra_csi *csi,
> > >  	for (i = 0; i < chan->numgangports; i++)
> > >  		chan->csi_port_nums[i] = port_num + i * CSI_PORTS_PER_BRICK;
> > >  
> > > -	chan->of_node = node;
> > > +	chan->of_node = of_node_get(node);
> > >  	chan->numpads = num_pads;
> > >  	if (num_pads & 0x2) {
> > >  		chan->pads[0].flags = MEDIA_PAD_FL_SINK;
> > > @@ -640,6 +640,7 @@ static void tegra_csi_channels_cleanup(struct tegra_csi *csi)
> > >  			media_entity_cleanup(&subdev->entity);
> > >  		}
> > >  
> > > +		of_node_put(chan->of_node);
> > >  		list_del(&chan->list);
> > >  		kfree(chan);  
> > 
> > Not related to your patch, but this kind of "one function cleans up
> > everything" style is always buggy.  For example, here it should be:
> > 
> > -		if (chan->mipi)
> > +		if (!IS_ERR_OR_NULL(chan->mipi))
> > 			tegra_mipi_free(chan->mipi);
> 
> I sort of agree the code could be clearer here, but looking at the code
> in detail, this cannot happen. chan->mipi is set in one place only, and
> if it is an error the whole probe fails. So it can be either NULL or a
> valid pointer here.

I assumed that tegra_csi_channels_cleanup() would clean up if
tegra_csi_channel_alloc() fails.  Otherwise then that's several
different even worse bugs.

HINT: Let's all hope my initial analysis was correct.

> 
> Regarding my patch, do you think it is valid?

It sort of depends on if the "chan->mipi" dereference can crash like I
explained above.  If there is no ->mipi crash then the error handling
needs to be fixed.

regards,
dan carpenter