Date:   Fri, 17 Dec 2021 12:28:17 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Steven Price <steven.price@arm.com>
Cc:     Rob Herring <robh@kernel.org>, Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Tomeu Vizoso <tomeu.vizoso@collabora.com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
        Boris Brezillon <boris.brezillon@collabora.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()
Message-ID: <20211217092817.GH1978@kadam>
References: <20211216161603.983711-1-steven.price@arm.com>
 <CAL_JsqKZBsJxy8h5EQf0wwfioWS-Kx9i=0cQ7p4FHckEXstGiw@mail.gmail.com>
 <4c564c0d-7702-9dfe-910f-969fe130aba8@arm.com>
 <20211217091046.GG1978@kadam>
 <4e56a7a8-c48c-49dc-6535-730ad871d1e1@arm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4e56a7a8-c48c-49dc-6535-730ad871d1e1@arm.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?w9/rCFRlPvZr8E8Sm3hCaVbifTQqaeG6TphyGAR1Cc3pTSFEFIFsyBrayifX?=
 =?us-ascii?Q?T94Qb+aU+if0RDW7R7MatwGpU50Tyow2lILunIXRPUFDVyVfj53YhXILOmgD?=
 =?us-ascii?Q?U3lRdruhNDD1Aa0UUzLkHXGJjxR227CP+94Fz6tp8sSHccYqPIDyW2JnsLwv?=
 =?us-ascii?Q?qHG9LL4db8s5Rdp8Ur5rDzWR38nnd5pr7B9d3noX3LBRCwlV6H6qb92lHoZe?=
 =?us-ascii?Q?R6JN5jG6UD3OIa6iY7ioD52V3FUOyrceqmUxgb4mxdgAFAgyu2ys3oWRtl7F?=
 =?us-ascii?Q?msbbGEIr+sZ5ITcPFya9YkVidDZPS5U9reyHUbZAdemA3znLjEhCkU26NMh3?=
 =?us-ascii?Q?Q7u4jEjrtGA1uN7WsNZmngoQdSMdAsuWOrF3lQgDH0LcW5abgIqfGUiNIj/+?=
 =?us-ascii?Q?KbPOx/dLha/G6Vn3ZbuC7vLfWSaM/MzWVS/ninSDuI3GpsCZCmpQJy3KNSBo?=
 =?us-ascii?Q?41o08/+1m038/0lcNFP3XpZlyXn2j3mI+iUeyj+Tmdx/Py19E8VcOGghfez5?=
 =?us-ascii?Q?EXkNAq+k/OKndivgU7c07WJFSfpU1QEsEv2Get2eYXhp5a1Yf70+2AiRVA0C?=
 =?us-ascii?Q?QB8RvqgvppV+vNInjXV57mHHIb6tv0dIF3HqC4FJ+ZpHrgQYqzsb2EbHuA5o?=
 =?us-ascii?Q?Bk/drfDvqsuL4JD44qIefHIZuevBq3GrALCsllAb7Z9GvnVa4zNC2hOHi9gR?=
 =?us-ascii?Q?wrAtkYHwXGI7VD1cC8xfOBMHcGvU11Ph9EFDZ6g8K6M/78DyOBFElj+lNJ7u?=
 =?us-ascii?Q?z23pJSZy//ok14IKBZkRZ98bYKqbmJ6RtmlVSLBb5QvLRoE+p61VvNbqKUif?=
 =?us-ascii?Q?zXUwXiQ9hJRaAhur3PUFisah/6hSBOhIlgs7Ky6wsTHtXkSIkKi5vyUhn9F1?=
 =?us-ascii?Q?66l41/Gr67CL0FJqXVYY7FAWQTx+hztttPB4uAnd8H2mS7hQ50iobwBozSyu?=
 =?us-ascii?Q?CU5UMbL7GBwxvFVtrxKBPV3kc4y8W18U2Km7iELvRr+pFuS9/ZGwmknzZDZ4?=
 =?us-ascii?Q?QfCr8cyhbYJ3KoA3iOYwHdDSDT1yRvLUbB4eCmI+LcRd87beSNHcFVIfb+5d?=
 =?us-ascii?Q?aFFIVHY2ISyvsWPDTY9ahngBnybdjDv7ROF+zSwp8z96sNVWG+27udGsdzC9?=
 =?us-ascii?Q?hZByWhm7ZelYCgawJcr++OtoQ5ay+wWzwqzGgWDFAkKXtJD4mYWhaVTyblxK?=
 =?us-ascii?Q?AQ+rOc6rz4ffnlfNmZxCHAsSQqloDOEO8XKa0THnLIJ25+pkExxF8QAzv/ua?=
 =?us-ascii?Q?g1ZeyqIU9PypGiisCVlEAfznHIYPIR/P+2qTgHg4n6lmWVA+F6MQNHF8cTxz?=
 =?us-ascii?Q?EcKrRG8777vmMnoilRMOjnOu5iHanQd66KgR7sHElCdhu6o/xNgFMnV/eGl6?=
 =?us-ascii?Q?sutgFQ9UNmSFwezunViE4IjqOzbK+0oemOi4Zc1yq0TdutB2bmTV/UUj75QR?=
 =?us-ascii?Q?l5BMOsj9Ck9ASOH8msjbUn0rA2kBwsnHy4yB1C9XxO5ptxBgLpAn7BzVGQxh?=
 =?us-ascii?Q?haOyHgUQR0KZI8STPA42PZrNnBASAvjCvLKuA4uDqPrgnoQJv6ny6Wm+cYZD?=
 =?us-ascii?Q?mpxkJ2xmP1eSQZiazRSce3rUosTiffBSdDDQHZXqvjPwbtbk3uUxF/hcC+uo?=
 =?us-ascii?Q?SGeZW8QkzLFm7Oc7wCP8HHA51zoOGA9EPj/X7cHm6qNIoXQxorDtLDjzIz0L?=
 =?us-ascii?Q?wRHJJ3ssLEXAlAmbLltov2WRtic=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8d14dd3c-5cdc-4d07-a481-08d9c13f9ba0
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Dec 2021 09:28:39.1880
 (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: OTn7BxqopJEc5ZkiKndBk5K0y4ENee8qkX/t/ZZd0udz5NuDYpyWJ2W5CgrPpFtfWTfmG0QPu7nXKQrcwdfzkAZtsOoPDFSeEkN16ETnowE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1488
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10200 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 spamscore=0
 mlxlogscore=999 suspectscore=0 adultscore=0 malwarescore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112170053
X-Proofpoint-ORIG-GUID: TqCtT2QomP1QsVerGC-uLQzHi0uw_6GZ
X-Proofpoint-GUID: TqCtT2QomP1QsVerGC-uLQzHi0uw_6GZ
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Dec 17, 2021 at 09:16:19AM +0000, Steven Price wrote:
> On 17/12/2021 09:10, Dan Carpenter wrote:
> > On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
> >> However this one is harder to fix without setting an arbitrary cap on
> >> the number of BOs during a sumbit. I'm not sure how other drivers handle
> >> this - the ones I've looked at so far all have the same issue. There's
> >> obviously the list that Dan already sent, but e.g. msm has the same bug
> >> in msm_gem_submit.c:submit_create() with an amusing bug where the check
> >> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
> >> large allocations are going to fail anyway.
> > 
> > sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
> > does work to prevent an integer overflow on 32bit systems.  But it's not
> > beautiful.
> 
> sz is the result of struct_size() which returns a size_t, and SIZE_MAX
> in case of an overflow.

Correct.

> However the check is *greater than* SIZE_MAX
> which will never occur even on 32 bit systems.

You've missed a part.  We add ((u64)nr_cmds * sizeof(submit->cmd[0]))
to SIZE_MAX.  If nr_cmds is zero then, whatever, the kzmalloc() will
fail.  No big deal.  But if it's non-zero then "sz" is larger than
SIZE_MAX and we allocate a smaller buffer than expected leading to
memory corruption.

Btw, it turns out that I had a hand in writing that check so hooray for
me.  :)  #HoorayForMe

regards,
dan carpenter