| Message ID | 20241106184935.294513-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: adv7511: ADV7535 fixes | expand |
Hi Biju, On Wed, Nov 6, 2024 at 7:49 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > The host_node pointer assigned and freed in adv7533_parse_dt() > and later adv7533_attach_dsi() uses the same. Fix this issue > by freeing the host_node in adv7533_attach_dsi() instead of > adv7533_parse_dt(). > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") > Cc: stable@vger.kernel.org > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > Changes in v3: > - Replace __free construct with readable of_node_put(). > Changes in v2: > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > - Dropped Archit Taneja invalid Mail address Thanks for the update! > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -143,6 +143,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) > }; > > host = of_find_mipi_dsi_host_by_node(adv->host_node); > + of_node_put(adv->host_node); This still looks fragile to me, as afterwards a pointer to a freed node is left in struct adv7511.host_node. It would be safer to also clear adv->host_node here. However, taking a look from a distance, the code looks like: static int adv7511_probe(struct i2c_client *i2c) { ... adv7533_parse_dt(dev->of_node, adv7511); ... if (adv7511->info->has_dsi) ret = adv7533_attach_dsi(adv7511); ... } The only reason adv7511.host_node exists is to pass it from adv7533_parse_dt() to adv7533_attach_dsi(). So what about making this explicit? 1. Let adv7533_parse_dt() return the host_node or an error pointer, 2. Pass the host_node as a parameter to adv7533_attach_dsi(), 3. Call of_node_put() in adv7511_probe() after use. > if (!host) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to find dsi host\n"); > @@ -181,8 +182,6 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > if (!adv->host_node) > return -ENODEV; > > - of_node_put(adv->host_node); > - > adv->use_timing_gen = !of_property_read_bool(np, > "adi,disable-timing-generator"); Gr{oetje,eeting}s, Geert
On Wed, Nov 06, 2024 at 08:04:16PM +0100, Geert Uytterhoeven wrote: > Hi Biju, > > On Wed, Nov 6, 2024 at 7:49 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > The host_node pointer assigned and freed in adv7533_parse_dt() > > and later adv7533_attach_dsi() uses the same. Fix this issue > > by freeing the host_node in adv7533_attach_dsi() instead of > > adv7533_parse_dt(). > > > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") > > Cc: stable@vger.kernel.org > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > Changes in v3: > > - Replace __free construct with readable of_node_put(). > > Changes in v2: > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > - Dropped Archit Taneja invalid Mail address > > Thanks for the update! > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > @@ -143,6 +143,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) > > }; > > > > host = of_find_mipi_dsi_host_by_node(adv->host_node); > > + of_node_put(adv->host_node); > > This still looks fragile to me, as afterwards a pointer to a freed > node is left in struct adv7511.host_node. It would be safer to also > clear adv->host_node here. > > However, taking a look from a distance, the code looks like: > > static int adv7511_probe(struct i2c_client *i2c) > { > ... > adv7533_parse_dt(dev->of_node, adv7511); > ... > if (adv7511->info->has_dsi) > ret = adv7533_attach_dsi(adv7511); > ... > } > > The only reason adv7511.host_node exists is to pass it from > adv7533_parse_dt() to adv7533_attach_dsi(). > So what about making this explicit? > 1. Let adv7533_parse_dt() return the host_node or an error pointer, > 2. Pass the host_node as a parameter to adv7533_attach_dsi(), > 3. Call of_node_put() in adv7511_probe() after use. Or, alternatively, if keeping the node pointer in the adv7511 structure is preferred, call of_node_put() at .remove() time. > > if (!host) > > return dev_err_probe(dev, -EPROBE_DEFER, > > "failed to find dsi host\n"); > > @@ -181,8 +182,6 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > if (!adv->host_node) > > return -ENODEV; > > > > - of_node_put(adv->host_node); > > - > > adv->use_timing_gen = !of_property_read_bool(np, > > "adi,disable-timing-generator");
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 06 November 2024 19:33 > Subject: Re: [PATCH v3 1/3] drm: adv7511: Fix use-after-free in adv7533_attach_dsi() > > On Wed, Nov 06, 2024 at 08:04:16PM +0100, Geert Uytterhoeven wrote: > > Hi Biju, > > > > On Wed, Nov 6, 2024 at 7:49 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > The host_node pointer assigned and freed in adv7533_parse_dt() and > > > later adv7533_attach_dsi() uses the same. Fix this issue by freeing > > > the host_node in adv7533_attach_dsi() instead of adv7533_parse_dt(). > > > > > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI > > > device") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > Changes in v3: > > > - Replace __free construct with readable of_node_put(). > > > Changes in v2: > > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > > - Dropped Archit Taneja invalid Mail address > > > > Thanks for the update! > > > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > @@ -143,6 +143,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) > > > }; > > > > > > host = of_find_mipi_dsi_host_by_node(adv->host_node); > > > + of_node_put(adv->host_node); > > > > This still looks fragile to me, as afterwards a pointer to a freed > > node is left in struct adv7511.host_node. It would be safer to also > > clear adv->host_node here. > > > > However, taking a look from a distance, the code looks like: > > > > static int adv7511_probe(struct i2c_client *i2c) > > { > > ... > > adv7533_parse_dt(dev->of_node, adv7511); > > ... > > if (adv7511->info->has_dsi) > > ret = adv7533_attach_dsi(adv7511); > > ... > > } > > > > The only reason adv7511.host_node exists is to pass it from > > adv7533_parse_dt() to adv7533_attach_dsi(). > > So what about making this explicit? > > 1. Let adv7533_parse_dt() return the host_node or an error pointer, > > 2. Pass the host_node as a parameter to adv7533_attach_dsi(), > > 3. Call of_node_put() in adv7511_probe() after use. > > Or, alternatively, if keeping the node pointer in the adv7511 structure is preferred, call > of_node_put() at .remove() time. We don't need to store unnecessary pointer if we handle the host_node pointer in the probe(). I will go with Geert's suggestion. Cheers, Biju > > > > if (!host) > > > return dev_err_probe(dev, -EPROBE_DEFER, > > > "failed to find dsi host\n"); > > > @@ -181,8 +182,6 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > > if (!adv->host_node) > > > return -ENODEV; > > > > > > - of_node_put(adv->host_node); > > > - > > > adv->use_timing_gen = !of_property_read_bool(np, > > > > > > "adi,disable-timing-generator"); > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 4481489aaf5e..de55d687245a 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -143,6 +143,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) }; host = of_find_mipi_dsi_host_by_node(adv->host_node); + of_node_put(adv->host_node); if (!host) return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n"); @@ -181,8 +182,6 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) if (!adv->host_node) return -ENODEV; - of_node_put(adv->host_node); - adv->use_timing_gen = !of_property_read_bool(np, "adi,disable-timing-generator");
The host_node pointer assigned and freed in adv7533_parse_dt() and later adv7533_attach_dsi() uses the same. Fix this issue by freeing the host_node in adv7533_attach_dsi() instead of adv7533_parse_dt(). Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") Cc: stable@vger.kernel.org Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- Changes in v3: - Replace __free construct with readable of_node_put(). Changes in v2: - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. - Dropped Archit Taneja invalid Mail address --- drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)