Date:   Thu, 16 Feb 2023 16:24:58 +0100
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Alok Tiwari <alok.a.tiwari@oracle.com>
CC:     <netdev@vger.kernel.org>, <darren.kenny@oracle.com>,
        <linux-kernel@vger.kernel.org>, <jhs@mojatatu.com>,
        <xiyou.wangcong@gmail.com>, <jiri@resnulli.us>, <kuba@kernel.org>,
        <simon.horman@corigine.com>, <keescook@chromium.org>
Subject: Re: [PATCH] net: sched: sch: null pointer dereference in
 htb_offload_move_qdisc()
Message-ID: <Y+5KyhntRjMzDM2G@lincoln>
References: <20230216104939.3553390-1-alok.a.tiwari@oracle.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230216104939.3553390-1-alok.a.tiwari@oracle.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BHI5Tu9Zw3FR26HC/2oKja8ZbBrV+5nMz+xPIhenpHoicoFqT7FKokWoGxbk?=
 =?us-ascii?Q?G6proA6XXG9W3RKapeDREYTl0uM8AmTHo3FfUvf5Cx76d6lxmsXqGtCBvKvB?=
 =?us-ascii?Q?+XTWKtBQXmxKicNxUTOpr+MfuKTiSUeMeGgagAqw3rwq942EMkqISCdEzJx7?=
 =?us-ascii?Q?2Ej21glL7DGJool7bz1DOAiWVVbWvGquzJBPzvyrKZIXAH0AWwocGuy9B14F?=
 =?us-ascii?Q?6nJGpiQ02O4InMJz4D5U9KdohW0KXafgdkaGGDWUxXKMKByJuCH5iPgNwVFG?=
 =?us-ascii?Q?5t5DlR1m2or9nxNqsYYHsI039r9w0b9+dXGk8+VVSAGl0Wvra+yNLXhlaVdR?=
 =?us-ascii?Q?xYRUa4zKwvrHK4wBYShkH2USeAAQjC5zj+p3aSPMeaLQ7zvwSFFLOzqhQNr6?=
 =?us-ascii?Q?P64gvA3DR+2ISHCI7woH21L9zR01uCRA1pWKq+Kgmsv0HVbzAijCejrTBQoo?=
 =?us-ascii?Q?PDTmfdYDFyXYdDEs3lJEnYzl2HKa3xXvjWEbAy8KePF2r7gtOBswNPdoOBt3?=
 =?us-ascii?Q?rB+xibJOvjLXhsIbeUcHfANMdxaFbG6f56dIBBjzwuNnLcub2734f2tzUUMN?=
 =?us-ascii?Q?1Bl8MIYmQmQ935aXEWOq63gjoiksHJwl9pvtK+bDdwb88GRo63eEOVyNwFyA?=
 =?us-ascii?Q?y/Ybk9yqK2ElT1b4sHzHJ6FZASm7Jp9FhPlGfFRpCJ2/A7Pl6CO2NgtC4zUi?=
 =?us-ascii?Q?6Inm0YVBqR5+v2GzkFF+gXnVSmHfBbFX6/48wwP3+WMSXgTstRQ958KXvTkq?=
 =?us-ascii?Q?yMhjpBiGD6jW3A5EsraCn3uHOFP3f/7sQ/d38PMT1EEn0q6zkkpTTdJI0zq3?=
 =?us-ascii?Q?93IeDZmGUUKM7GjPLVLjGU9xmJ+08dh6jLhc89r6E/e1HJdJB5GB/YNNHn2O?=
 =?us-ascii?Q?suxr+tELW/1pDrCiyptLxSkcsHUf2ClrCqgzrmEoB1W6BuS2JZfECL7jhA7Z?=
 =?us-ascii?Q?fwkxsX3rGuL/nitjHOZ5055X24qDi+/vTKZjRbxloXzwu2d7PHRTkOoyy9F0?=
 =?us-ascii?Q?bHlTKUGro+w688ESXXWeeS4iT2Be6KWYTtNpdLjuPblWQZjNW+0ZCRUOVHh6?=
 =?us-ascii?Q?3sga9dfBqjQ/Wt24BwkVUZ9WzSlyhVOLHSXz4awi9W0RNHCmxULr+7edOe5f?=
 =?us-ascii?Q?BWIlqDp8JewWbeHW/1G0JeduW5G6lzz5trADrOREAvW3LCfkg/1eZ5Fk8a25?=
 =?us-ascii?Q?ZEqhe75Tk77fnokEj1v1MgiuJKZ06m9Kohrqd8JyYpYHa7oSY5jMMOwBQO4N?=
 =?us-ascii?Q?QGb8QT7LqZfxdCuKyfeBusV97ROj0ITNse5/Ozp8vYgJTuNK9Xx5Kb3Pbahw?=
 =?us-ascii?Q?A0P56UVMx4QDy64vH0ZQz3zotzPxKgBolswoJyYPDm+j48LcUx0Wc7lNxOE7?=
 =?us-ascii?Q?d7ObyAlqXQ0OrVwfP1y7LZ+CIK4camYPoSPv2AvTrAWCFfAc1sUcDmRGbDtg?=
 =?us-ascii?Q?qBDY55+wJOErl3GrCX+cfBhSDhYNKDGSyUjWBypiN0IR8d6W24jU+R0sik+D?=
 =?us-ascii?Q?+gWlnePk8iZGWCeGjLgLHjUMh9jzEtq9qvQCI2v59L9d89mQi+VwM7M+AZKd?=
 =?us-ascii?Q?ADtyd0CykmaNmo3X4DOJgNJnCXosdhBN7EQ0hw7gJtegTavpkZuzWdV5MDu6?=
 =?us-ascii?Q?jQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fcc422ca-d97a-44a6-a4d3-08db10348843
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5471.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2023 15:43:21.5410
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gsjX9MWmUZvrwJhJx2UTkYHNkICSPscfQlAtuf4AjwEB753U+5jauM+LmCWhgQ0bfrLroF1VlmG6Deyf4qOHQLW7zLdAhz5jPucFRVbj1HE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6413
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 16, 2023 at 02:49:40AM -0800, Alok Tiwari wrote:
> A possible case of null pointer dereference detected by static analyzer
> htb_destroy_class_offload() is calling htb_find() which can return NULL value
> for invalid class id, moved_cl=htb_find(classid, sch);
> in that case it should not pass 'moved_cl' to htb_offload_move_qdisc()
> if 'moved_cl' is NULL pointer return -EINVAL.
> 
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>

Conceptually, code looks fine, but it probably won't be accepted without:
- imperative verb in the title
- tree, to which patch must be applied [0]
- more nicely written and formatted commit message. If you are referring to a 
  static analyzer and not to common logic, maybe include, how the warning looks.
  Separate your text into sentences and paragraphs.

[0] https://www.kernel.org/doc/html/v5.3/networking/netdev-FAQ.html#q-how-do-i-indicate-which-tree-net-vs-net-next-my-patch-should-be-in

> ---
>  net/sched/sch_htb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 92f2975b6a82..5a96d9ea3221 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -1601,6 +1601,8 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
>  		u32 classid = TC_H_MAJ(sch->handle) |
>  			      TC_H_MIN(offload_opt.classid);
>  		struct htb_class *moved_cl = htb_find(classid, sch);
> +		if (WARN_ON_ONCE(!moved_cl))
> +			return -EINVAL;
>  
>  		htb_offload_move_qdisc(sch, moved_cl, cl, destroying);
>  	}
> -- 
> 2.39.1
>