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;
Message-ID: <7dc11da9-ddd5-d73e-ce76-7ecbd78b946a@oracle.com>
Date:   Mon, 28 Nov 2022 18:05:22 +0530
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.4.2
Subject: Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config()
Content-Language: en-US
To:     Stefano Garzarella <sgarzare@redhat.com>,
        Dan Carpenter <error27@gmail.com>
Cc:     harshit.m.mogalapalli@gmail.com,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Xie Yongji <xieyongji@bytedance.com>,
        Gautam Dawar <gautam.dawar@xilinx.com>,
        Parav Pandit <parav@nvidia.com>, Eli Cohen <elic@nvidia.com>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
References: <20221128083627.1199512-1-harshit.m.mogalapalli@oracle.com>
 <20221128105312.3ajursuudvmysiie@sgarzare-redhat> <Y4SUOPX2WRFiuB7n@kadam>
 <20221128111310.6exrqi26grwspqcz@sgarzare-redhat>
From:   Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <20221128111310.6exrqi26grwspqcz@sgarzare-redhat>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eWpTN0hmRFFPU3BVM3E4TWhMbEZRWkJrQUxWU1JpSFljaHJtL3piMjU4ZG1F?=
 =?utf-8?B?cHE1VTVZL280RkVGQUorK1JpaW5tU3BzTHZSR0Jtb29hOEREVGZnaUpHamV3?=
 =?utf-8?B?TG0vVjB2RElqSGtOR1ppVHRxQmUvbmgwQU1ZQ3QzUkJwcXlWRDh0ZWlDN2Jk?=
 =?utf-8?B?dkNOcmZUTlFvSGw3alhsS3lwVGh4T2t3bVFhUGZMMVA5enpLSVA1bWxJTDRO?=
 =?utf-8?B?Y0tjeUJITTVCV1p0cHlHdlJVODVCcmJqdURVVE5zaktXcGc2MnFpaFpTQTAw?=
 =?utf-8?B?N2tYbVlQUk15K3FXbko1Znk2NSsxU2FLL0drYmo0U3hETE5YSkxlbFA0NUlU?=
 =?utf-8?B?ckI4UldFQ00rWTRNa3VpbjN6Z0dtNThNdlEwQ1N1ck5PN0lNcWZUMllYRlcw?=
 =?utf-8?B?R2ZTcjBzeGFVZUo1akcvQ2pqYklVd2xTTnFna3hMMEY5Q1I3SnFGRXVPeGla?=
 =?utf-8?B?bzZwUHJOQlFqWnl1RnNFUytUWnVxbVpqUlUzbHlFRFpyR2Z0WmljR0ZxVi9v?=
 =?utf-8?B?YlU5TDNGQlR2VkZ4MDY5WHhpa08rOWYrQlRBbGkybGJjeDFVZFFlZ2RyaklB?=
 =?utf-8?B?RFlEUFhEZFFoNlRsSVJURnBuY1RldDIrU0FWU2RPN0Rwd2Q3V3k1a2JhMFpR?=
 =?utf-8?B?SVNTbDNjRnBwd3RNQlRwdy92d3piVTNsT1Z5YjVlMmpNRGJpb1F5dE5xMVB0?=
 =?utf-8?B?SGpxang2TUlpMlIzZkMwUkFZaW9XZWRucXV4UVNlRUdWYk5jWXBYN3VmcTM4?=
 =?utf-8?B?U0ZFRVlBZUdzaEdVZUxrTTZ0VitXdDNKYnlaM1BXY2lnNmtJK2xpYS9YL1VI?=
 =?utf-8?B?VFRYNmo0MFVkQk84Q2VUQU9aNWUrZ1pFZ24rUVhhYmIzMmIzUms3NmttNGdr?=
 =?utf-8?B?T1ZmeGM3RnJBVXpEV1lpc3J5YzBMbDVmWkd0dTBuNDdGUEN1MzZtTmd3REFs?=
 =?utf-8?B?SElkNDlyZWsxbEFTWVNBNThpSjRqalV3RzExT0k3aFl5aVcyNjZnSU50Mjlo?=
 =?utf-8?B?SmlDWGZMaW12Zk1zaU9BZ0xvOFVXZHlCK2U3bFpSVHRXNTdHTlNNRmJEVnpS?=
 =?utf-8?B?U2k3SEpWY2Rva3ZjTkUyODlmT2NTSEtXUlEyRjBjOHVrb010SG92NFNMMTVm?=
 =?utf-8?B?cE1zdStva1pPUy9wdmNHVWVUc0tRRzhmdENwR3czTEJtRDdrLzRUaTNBRURn?=
 =?utf-8?B?aTNVZTR4TXpBOVVjMmNvWlFGa0NUemE4N25KeklMYTlHZWFuQnpuWklFWWlU?=
 =?utf-8?B?K1I5Y3p4aGtWYVN0VVhuYW5DeVZVR2ZLd21ybWVRbWpqZTgyYllzN2NyWmpB?=
 =?utf-8?B?S2Z6VVgvVHJ5bTYzTTh1NEFVRVhpRHBOU0lhMFl0dGVpZEFENnJoTGdLZUsw?=
 =?utf-8?B?QTUzZ2NQMC9oYTNEK1VaZ1k5ZEVQUkxnWmo4dnZJWGdNTlRYN08zOVd0eUE3?=
 =?utf-8?B?Z0d3TG16N2g1bUZyUlVIckZUQy9NamlBTzVpVmNWSThnVTg0T0lWRnpZWms3?=
 =?utf-8?B?MDNWS25uditJRTRwUkhrNXF5NDdkT0F6WXJVcDQ2RFRCM2dGVEdhS2x0alpy?=
 =?utf-8?B?YWNPeXRCV3RxZzIvakwrY0lib0xxYitkcFJWMmZXV0NWR21CTGlnMmtxeHpU?=
 =?utf-8?B?RSs3a1BNMzQ2bFRaYUhPbGpNWTRrWkNjQ1NLQ3ZRcDQ4OXFqQXpiNE94K0Nt?=
 =?utf-8?B?SzFQaU1ydXUvdUU3aFVTbS9SdkZqR2NIOG83T05IN2FNaHVrN1RBYzA3bnha?=
 =?utf-8?B?QkJUWWMwOEkvdUVvZmc1ZFFESkFmbmVhdHZ1NjN5THRMVjJxb0MrSkJkSERz?=
 =?utf-8?B?aW9hVGJydEJ2Mm01L3cvQThvVWgybVQ1aVBxOWF1bWJUbjlZcTd5cUZxMjRy?=
 =?utf-8?B?MkRubHovRjNYTm5lQUpxc2JrcncwOGtxVEtKeXE4Z1FaajZlVVBnYnlNbk9K?=
 =?utf-8?B?RmZ3enBRZjl1Z0hUOVo1anN5T0tqTXBTY0NncEVWMGRDMDQ0VU1xbU9WQW1n?=
 =?utf-8?B?TDhaYjFtTHdWcFRJZ3BlVGNLUVB0VnVyRE1nbFg4RkJyZW9QS3prYnBwWW13?=
 =?utf-8?B?MXBBR0twcCtiVW1SOFQ2QjdFSUF6a2VzY0d2aVFzbEpCYWpCZ2VHMUxpQ3Ar?=
 =?utf-8?B?b3Q2U2xIVXNpWllGcWpUY05xaVJkOWFPTXJWTHArWUxPdjhmckFiMmJVWTl1?=
 =?utf-8?Q?LFwzyUfUTZ8E7fXNOQNS42g=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?NEZ6cWhVM3hzUnFFbzlMcWoySGh1SThOVEZZOFY4ajZ1WlhzVDl0S0RPRUhx?=
 =?utf-8?B?ZTRyWU1wL0gycW9LVldkSlQzbVJMM2dMUDBuakZSVlRzYWh6VWkvQ3pnUzhY?=
 =?utf-8?B?OHE5UWdEV29uZHN4YnBKMHd1WHlvSVN4NnJYb0pITFdRNXFLdGk3STd4WlBs?=
 =?utf-8?B?ajhiak9hTHJLVnlFVzJaUXZ0amNwTVJkelRaWnBtYUFsbTdBbmlGWUdIVWVv?=
 =?utf-8?B?OC9sTFl1MVI1YTJvT25xdGYwS0NvTmNoOW9yQzUvM0pwNWI2bkU1SHdSVnJM?=
 =?utf-8?B?MDU2QVhBNERadVgwWHdwWnBtaGpvSUNCVWtBVDI2cWlhNjF6U281V0t2cVJa?=
 =?utf-8?B?WkR5OFVXeGZoWlA0WlJEVUFuSUFYWGVkRElJOEZyY01vd29TZHBIeGpSZ2Rk?=
 =?utf-8?B?Y1JxOHZ5ckhVdms5UzFzQXNVSGZrOTdxZWxFSktIOVg5TElsMTlLdEVlMm1F?=
 =?utf-8?B?RWhtVTVVSHZST0YvNHB6eEo1K2VBTXcyNXVRN0NyWjNxNmFjOWNJSGtGUDVk?=
 =?utf-8?B?TU1KRHhuemtiZndyLy9BUVRrVThoWStCOTFXdC9iU2pIanZXdTRrM3Z0bEE0?=
 =?utf-8?B?QmV5Q1dMQUo1bFlpdlpuSnlLd3ZYNnNsaFp0WXBRc0V3Q29kSURDOURIS2lT?=
 =?utf-8?B?TjdHTTVMbUU2R0JtMGM0czFieUJGMnJKUHJlTDk1OHBiMTlIdE1xY21VNmtL?=
 =?utf-8?B?TTB6a1J1VmUrZ3Z5RHJ4Vi9ZR2lqQ0JEUHVHV1NhanRWd3MvMk9WdGNBRzBu?=
 =?utf-8?B?YnpiVHA2MnMrQk0xdVFpQlBmOTBHdmNTRFJNTjVkQkY1bjFxUCtva3FxRnF0?=
 =?utf-8?B?VGRZdmllZ2hZY0xZT2dBVXZvdG9pK3ZkNmdlS1EvNzY5WTlHdWFKMmoyRmVF?=
 =?utf-8?B?RWs5WVdPUFJiOC83M1FaQzEzK3k1eGZ3Z2Y2VjdNOHQ5d2pQa3BFTTlKaVN5?=
 =?utf-8?B?NXM4Tm8xUGd4QjNZcjZxNUZBeitnbHBGdm9iLzlyMnp4dzN0VUplMW1uNjYy?=
 =?utf-8?B?VlR6NkpCRUVYdVNVWHhRbGdETHFpVk9ZaFZGSjFyWWRhdFNOUjJaanFCdnUv?=
 =?utf-8?B?S2tIQ0l2OWZkd09mODdCYTZnaTRLRksrS3lPd2hWejZTTFc5VlVndzBCWEc2?=
 =?utf-8?B?dmJFY29VeklzdlFBT1lCQjI4QUFtMmlUWGN4MTU1OFQ5TzJZblFaRzE3S3E1?=
 =?utf-8?B?dDZJWUlnTzRaSkxXUUUrbTZxekNNTkh2a2dDeVBNbVUxV1h3ZVpHQksxTlQx?=
 =?utf-8?B?cWttcHFkTk5QcUNlMGhaemdmVU94NnFnQnk2OFRvOFpXeXJhcE1Nd28va1g3?=
 =?utf-8?B?ZnNsSGlhVTJZVWp5WUhWbG5YU2pFdFd1emtBUWVvSHRFeFJQWmpLSXloeUdO?=
 =?utf-8?B?ZThEUHNzbXVvR1JLS1lwWDcxa0FOUEc2c1lqMVhhaWJSVlpjV1JCVC9BWGov?=
 =?utf-8?B?cHVIT1Q2bHN1OWpzNDlZN1I1b29NQllEYmp6UFFBbDdmbmNrZlBxUkZJQnNC?=
 =?utf-8?Q?O6DUbU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8a4e95b2-f3ba-48b2-289f-08dad13d0b16
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6290.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Nov 2022 12:35:33.8381
 (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: CxQyPMZUhVWRVN7ZPuCGZW7+AvXcvJizwRjLOxvuqljbz/vV68zTBj3ZBalC7I6Q38INp20cBLykhZMUVGtFcJfPGaZW9uCmNOClUIIOClTst7GDGa4q7rJlRDjSxrmU
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB5823
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-28_10,2022-11-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 suspectscore=0
 bulkscore=0 adultscore=0 mlxlogscore=999 malwarescore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211280096
X-Proofpoint-GUID: -_T4VMPrgebqVfUMeL_XysUALASxWWe6
X-Proofpoint-ORIG-GUID: -_T4VMPrgebqVfUMeL_XysUALASxWWe6
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
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


On 28/11/22 4:43 pm, Stefano Garzarella wrote:
> On Mon, Nov 28, 2022 at 01:58:00PM +0300, Dan Carpenter wrote:
>> On Mon, Nov 28, 2022 at 11:53:12AM +0100, Stefano Garzarella wrote:
>>> On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote:
>>> > Add a limit to 'config->vq_num' which is user controlled data which
>>> > comes from an vduse_ioctl to prevent large memory allocations.
>>> >
>>> > This is found using static analysis with smatch.
>>> >
>>> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>> > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>>> > ---
>>> > v1->v2: Change title of the commit and description, add a limit to
>>> >     vq_num.
>>> >
>>> > Note: I think here 0xffff is the max size of vring =  no: of vqueues.
>>> > Only compile and boot tested.
>>> > ---
>>> > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++
>>> > 1 file changed, 3 insertions(+)
>>> >
>>> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
>>> b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> > index 35dceee3ed56..31017ebc4d7c 100644
>>> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> > @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct 
>>> vduse_dev_config *config)
>>> >     if (config->config_size > PAGE_SIZE)
>>> >         return false;
>>> >
>>> > +    if (config->vq_num > 0xffff)
>>>
>>> What about using U16_MAX here?
>>
>> Where is the ->vq_num stored in a u16?  I looked for this but didn't
>> see it.
> 
> I thought vq_num referred to the number of elements in the vq (like 
> .get_vq_num_max), since this patch wants to limit to 0xffff.
> 
> But it actually refers to the number of virtqueue, so @Harshit why do we 
> limit it to 0xffff?
> 

Hi Stefano,

I may be incorrect about the details of this driver, my v1 was purely 
based on static analysis, Micheal suggested me to put a limit of 0xffff 
on vq_num. I really don't know about the driver, while I was searching 
other parts of code, I thought 0xffff is based vring size, I have asked 
the same question on v1 today.

Ref to v1: 
https://lore.kernel.org/all/82e8ce27-0743-59bf-fbe8-a25093167451@oracle.com/

> Maybe we should explain it in the commit message or in a comment.
> 

yeah, we should clarify the limit in commit message, once Micheal shares 
about the limit '0xffff), I will add those details and send a next 
version if that's okay.


Thanks,
Harshit
> Thanks,
> Stefano
>