Message ID | 20221029093413.546103-1-jose.exposito89@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1266000wru; Sat, 29 Oct 2022 02:35:38 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7MvsLvZHDynLB5vJuizPHHrtCIOlmQoMtV5ZTXwoesQYpQ9NqAloNNBQpXZLMsuP2o6Tew X-Received: by 2002:a05:6a00:e1b:b0:537:7c74:c405 with SMTP id bq27-20020a056a000e1b00b005377c74c405mr3527394pfb.43.1667036138141; Sat, 29 Oct 2022 02:35:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667036138; cv=none; d=google.com; s=arc-20160816; b=eDRQxKrD4c3J9xNy8Ehb2hNEJWoqfd6CSq5WzmUHSFihmXsfKO5RfsD4I1xYdYsaRL pdqIiRwg3PdJOBMDtg5uUOdXNxLZOB6Op04CMVBrbw8ffJLyrLwrw8Jv818RfD82Tt5C eWuMcP8/aMkuJwn3XcbMnyww/VgpzLPLzBUdX8H5bFmu/g+Vvh0ElUAsAWa9Nt0LwTzV etUUXbh/95TaH5NmAytijCW6/idhSWKHJ3fYEeGGB31WazDwj1ledJS0Bw8djvjqEHWa vNa6rf3zczSXrUvEvkN3GzaKiMJvOFdyjSRbVXk1/1Df6NQ3GHiQgWzIGTWWb0kr3B3Q hKNA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=0s7cusbjRhvfSFjBw6Lx4DFnE28qr99VM/89xctrXcY=; b=dpYDzRrqMUTdEsfeZjNmN5mkTCIguEwyD5YkNwGJmXJxpAj+Il1CrHTFdepKJxjQtN GPjtI15yCPRbKY9O0GwdlhN/k1sQFx5iQmHSl6NHou+uMAzCcCzEkNftifJ+oy/3NNeG jz386x42PsI28kUYIXAsO+yzzi9n/Pg37N8iofaI2jGBCckZkF+G6T0xWwdK8eNZdto6 9HST1/70HMSd1YPRcQGPSiTTsf9Buwf7o9eUhai0xxLrVkn8oZ2Y8XYsC5qnknLHOku8 OFE45D2Uyn+bM1bwjPSO20WKwMaNuoPxgMWoQxfnQW9zJB8bb1HBPOH9/mGebIdPVSGA r4MQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=R8r9MYiV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l67-20020a633e46000000b0046ec9d03aaasi1449616pga.589.2022.10.29.02.35.22; Sat, 29 Oct 2022 02:35:38 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=R8r9MYiV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229510AbiJ2Jer (ORCPT <rfc822;pusanteemu@gmail.com> + 99 others); Sat, 29 Oct 2022 05:34:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60640 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229456AbiJ2Jen (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 29 Oct 2022 05:34:43 -0400 Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8FDA117049 for <linux-kernel@vger.kernel.org>; Sat, 29 Oct 2022 02:34:39 -0700 (PDT) Received: by mail-wm1-x335.google.com with SMTP id v130-20020a1cac88000000b003bcde03bd44so7894208wme.5 for <linux-kernel@vger.kernel.org>; Sat, 29 Oct 2022 02:34:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=0s7cusbjRhvfSFjBw6Lx4DFnE28qr99VM/89xctrXcY=; b=R8r9MYiVR/n1QCShgiCRjvlsyCmGxIth8d68WOBKuFwGFdpvU0hLifsKHXaA0i7R7T FATSb70x39JDuCr+RFIEMHkzCbXWDBVXLQrDeb9y7eB4/OehDirtQMwmoI+nEp+BrN0G u/fUtaHezDnpUbrRdrgmORy9rD4FhXGYMCmjG/LapRowSvd+EZrEGPnCIBo1dsmqrQzE esyFj2o/J+WqlmQTU+aoX79Wvn1x2MTGTvXbk00TQxNzSp0U4mMc+L1GZbZv4260jlSX DI4iK78azGeF6BR5yBioI4MAexmAQiV7qakqP16N3f0ibROLougrJyoyhWfCUkuqRZ00 Lydw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0s7cusbjRhvfSFjBw6Lx4DFnE28qr99VM/89xctrXcY=; b=25vfB19PYz3EZ3QYszyFKnt+t5tvUPJGoYqCdemPIHjw832vt13thOqcpS8eYvWKk3 x1X5WVWQ9rc6BtV4EEb9medZKJKewLl+DVFZ+NR4/cRQ9gTdF6DUW9BiVLaXR8/tXBn9 LjCrWnc+D1A9Nn9uhECQVHYCW1tcWfX8k85V8/noigQqYz6SlFOqXmP+G4P/Zj9yT/QK PrtRzl69qsJs/VlUf0558g796YEyQeJ9dg2Z0Tz4k/bZ2CkwnKQVUSSDlQgafoXDyVEY UixinLOt/6n/9Eot07YmFBVTXdZOqNDWjcTt90jLaBbb2Nnb6wfAgx1RflkW0B/qGM+A izkw== X-Gm-Message-State: ACrzQf1UmkKnSbYEY6DW8r//rrqOq6yADbiXX/Z2bU2no+4IY1yF5yVz pO6L2CPzenbXFZdBKnsajCc= X-Received: by 2002:a05:600c:310f:b0:3c6:ff0a:c41 with SMTP id g15-20020a05600c310f00b003c6ff0a0c41mr11820655wmo.91.1667036078032; Sat, 29 Oct 2022 02:34:38 -0700 (PDT) Received: from localhost.localdomain ([94.73.35.109]) by smtp.gmail.com with ESMTPSA id k18-20020adfe3d2000000b00236705daefesm1053785wrm.39.2022.10.29.02.34.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 29 Oct 2022 02:34:37 -0700 (PDT) From: =?utf-8?b?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com> To: mripard@kernel.org Cc: emma@anholt.net, airlied@gmail.com, daniel@ffwll.ch, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, =?utf-8?b?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com> Subject: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check Date: Sat, 29 Oct 2022 11:34:13 +0200 Message-Id: <20221029093413.546103-1-jose.exposito89@gmail.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=1.5 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SBL_CSS,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Level: * 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748014085640209234?= X-GMAIL-MSGID: =?utf-8?q?1748014085640209234?= |
Series |
drm/vc4: hdmi: Fix pointer dereference before check
|
|
Commit Message
José Expósito
Oct. 29, 2022, 9:34 a.m. UTC
Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
the vc4_hdmi_reset_link() function. This function dereferences the
"connector" pointer before checking whether it is NULL or not.
Rework variable assignment to avoid this issue.
Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
Hi, On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote: > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced > the vc4_hdmi_reset_link() function. This function dereferences the > "connector" pointer before checking whether it is NULL or not. > > Rework variable assignment to avoid this issue. > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 4a73fafca51b..07d058b6afb7 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc, > static int vc4_hdmi_reset_link(struct drm_connector *connector, > struct drm_modeset_acquire_ctx *ctx) > { > - struct drm_device *drm = connector->dev; > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base; > + struct drm_device *drm; > + struct vc4_hdmi *vc4_hdmi; > + struct drm_encoder *encoder; > struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > struct drm_crtc *crtc; > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector, > if (!connector) > return 0; > > + drm = connector->dev; > + vc4_hdmi = connector_to_vc4_hdmi(connector); > + encoder = &vc4_hdmi->encoder.base; > + I don't think that's right. Connector shouldn't be NULL to begin with, how did you notice this? Maxime
Hi Maxime, Thanks a lot for looking into the patch. On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote: > Hi, > > On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote: > > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced > > the vc4_hdmi_reset_link() function. This function dereferences the > > "connector" pointer before checking whether it is NULL or not. > > > > Rework variable assignment to avoid this issue. > > > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 4a73fafca51b..07d058b6afb7 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc, > > static int vc4_hdmi_reset_link(struct drm_connector *connector, > > struct drm_modeset_acquire_ctx *ctx) > > { > > - struct drm_device *drm = connector->dev; > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base; > > + struct drm_device *drm; > > + struct vc4_hdmi *vc4_hdmi; > > + struct drm_encoder *encoder; > > struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > struct drm_crtc *crtc; > > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector, > > if (!connector) > > return 0; > > > > + drm = connector->dev; > > + vc4_hdmi = connector_to_vc4_hdmi(connector); > > + encoder = &vc4_hdmi->encoder.base; > > + > > I don't think that's right. Connector shouldn't be NULL to begin with, > how did you notice this? > > Maxime This issue was reported by Coverity. At the moment this function is not invoked with a NULL connector by any code path. However, since the NULL check is present, in my opinion, it makes sense to either remove it or make it usefull just in case the preconditions change in the future. But at the moment, this is not a big deal. Thanks, Jose
On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote: > Hi Maxime, > > Thanks a lot for looking into the patch. > > On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote: > > Hi, > > > > On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote: > > > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced > > > the vc4_hdmi_reset_link() function. This function dereferences the > > > "connector" pointer before checking whether it is NULL or not. > > > > > > Rework variable assignment to avoid this issue. > > > > > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") > > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index 4a73fafca51b..07d058b6afb7 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc, > > > static int vc4_hdmi_reset_link(struct drm_connector *connector, > > > struct drm_modeset_acquire_ctx *ctx) > > > { > > > - struct drm_device *drm = connector->dev; > > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base; > > > + struct drm_device *drm; > > > + struct vc4_hdmi *vc4_hdmi; > > > + struct drm_encoder *encoder; > > > struct drm_connector_state *conn_state; > > > struct drm_crtc_state *crtc_state; > > > struct drm_crtc *crtc; > > > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector, > > > if (!connector) > > > return 0; > > > > > > + drm = connector->dev; > > > + vc4_hdmi = connector_to_vc4_hdmi(connector); > > > + encoder = &vc4_hdmi->encoder.base; > > > + > > > > I don't think that's right. Connector shouldn't be NULL to begin with, > > how did you notice this? > > > > Maxime > > This issue was reported by Coverity. At the moment this function is not > invoked with a NULL connector by any code path. However, since the NULL > check is present, in my opinion, it makes sense to either remove it or > make it usefull just in case the preconditions change in the future. Yeah, it makes sense I'd ask for a small cosmetic change then, could you add the assignments where they are actually needed instead of at the top of the function? Something like if (!connector) return 0; +drm = connector->dev; ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx); ... +vc4_hdmi = connector_to_vc4_hdmi(connector); if (!vc4_hdmi_supports_scrambling(vc4_hdmi)) ... Changing the prototype of vc4_hdmi_supports_scrambling to take a struct vc4_hdmi pointer would also help, it's much more convenient. Maxime
On Mon, Nov 07, 2022 at 09:26:30AM +0100, Maxime Ripard wrote: > On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote: > > Hi Maxime, > > > > Thanks a lot for looking into the patch. > > > > On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote: > > > Hi, > > > > > > On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote: > > > > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced > > > > the vc4_hdmi_reset_link() function. This function dereferences the > > > > "connector" pointer before checking whether it is NULL or not. > > > > > > > > Rework variable assignment to avoid this issue. > > > > > > > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") > > > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > > > > --- > > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++--- > > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > > index 4a73fafca51b..07d058b6afb7 100644 > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc, > > > > static int vc4_hdmi_reset_link(struct drm_connector *connector, > > > > struct drm_modeset_acquire_ctx *ctx) > > > > { > > > > - struct drm_device *drm = connector->dev; > > > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base; > > > > + struct drm_device *drm; > > > > + struct vc4_hdmi *vc4_hdmi; > > > > + struct drm_encoder *encoder; > > > > struct drm_connector_state *conn_state; > > > > struct drm_crtc_state *crtc_state; > > > > struct drm_crtc *crtc; > > > > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector, > > > > if (!connector) > > > > return 0; > > > > > > > > + drm = connector->dev; > > > > + vc4_hdmi = connector_to_vc4_hdmi(connector); > > > > + encoder = &vc4_hdmi->encoder.base; > > > > + > > > > > > I don't think that's right. Connector shouldn't be NULL to begin with, > > > how did you notice this? > > > > > > Maxime > > > > This issue was reported by Coverity. At the moment this function is not > > invoked with a NULL connector by any code path. However, since the NULL > > check is present, in my opinion, it makes sense to either remove it or > > make it usefull just in case the preconditions change in the future. > > Yeah, it makes sense > > I'd ask for a small cosmetic change then, could you add the assignments > where they are actually needed instead of at the top of the function? > > Something like > > if (!connector) > return 0; Dunno why you want to keep around dead code like that. I'd just nuke the bogus null check. > > +drm = connector->dev; > ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx); > > ... > > +vc4_hdmi = connector_to_vc4_hdmi(connector); > if (!vc4_hdmi_supports_scrambling(vc4_hdmi)) > > ... > > Changing the prototype of vc4_hdmi_supports_scrambling to take a struct > vc4_hdmi pointer would also help, it's much more convenient. > > Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 4a73fafca51b..07d058b6afb7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc, static int vc4_hdmi_reset_link(struct drm_connector *connector, struct drm_modeset_acquire_ctx *ctx) { - struct drm_device *drm = connector->dev; - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); - struct drm_encoder *encoder = &vc4_hdmi->encoder.base; + struct drm_device *drm; + struct vc4_hdmi *vc4_hdmi; + struct drm_encoder *encoder; struct drm_connector_state *conn_state; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector, if (!connector) return 0; + drm = connector->dev; + vc4_hdmi = connector_to_vc4_hdmi(connector); + encoder = &vc4_hdmi->encoder.base; + ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx); if (ret) return ret;