| Message ID | 20241105111228.112813-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, Thank you for the patch. On Tue, Nov 05, 2024 at 11:12:18AM +0000, Biju Das 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 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(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > index 4481489aaf5e..3e57ba838e5e 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -133,6 +133,7 @@ int adv7533_patch_cec_registers(struct adv7511 *adv) > > int adv7533_attach_dsi(struct adv7511 *adv) > { > + struct device_node *np __free(device_node) = adv->host_node; This raises so many red flags. The function will free the node while the adv->host_node variable still points to it, opening the door to use-after-free. Furthermore, there's nothing in the function name that hints it will do this, callers can get surprised by this behaviour. I'm sure you can do better than this and fix the problem with readable code, not cryptic and error-prone constructs :-) > struct device *dev = &adv->i2c_main->dev; > struct mipi_dsi_host *host; > struct mipi_dsi_device *dsi; > @@ -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: 05 November 2024 16:02 > Subject: Re: [PATCH v2 1/2] drm: adv7511: Fix use-after-free in adv7533_attach_dsi() > > Hi Biju, > > Thank you for the patch. > > On Tue, Nov 05, 2024 at 11:12:18AM +0000, Biju Das 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 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(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > index 4481489aaf5e..3e57ba838e5e 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > @@ -133,6 +133,7 @@ int adv7533_patch_cec_registers(struct adv7511 > > *adv) > > > > int adv7533_attach_dsi(struct adv7511 *adv) { > > + struct device_node *np __free(device_node) = adv->host_node; > > This raises so many red flags. The function will free the node while the > adv->host_node variable still points to it, opening the door to I agree, adv->host_node still points to it and open again the door to use-after-free. > use-after-free. Furthermore, there's nothing in the function name that hints it will do this, callers > can get surprised by this behaviour. By looking at [1], it can free the node pointer when it returns from the function. As you said there is no hints from function name. __free(device_node) just tells to free the random device node assigned to the local variable. [1] https://elixir.bootlin.com/linux/v6.12-rc5/source/include/linux/of.h#L138 > > I'm sure you can do better than this and fix the problem with readable code, not cryptic and error- > prone constructs :-) Agreed. Will fix the problem with readable code. Cheers, Biju > > > struct device *dev = &adv->i2c_main->dev; > > struct mipi_dsi_host *host; > > struct mipi_dsi_device *dsi; > > @@ -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..3e57ba838e5e 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -133,6 +133,7 @@ int adv7533_patch_cec_registers(struct adv7511 *adv) int adv7533_attach_dsi(struct adv7511 *adv) { + struct device_node *np __free(device_node) = adv->host_node; struct device *dev = &adv->i2c_main->dev; struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; @@ -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 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(-)