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: <8253ab3f-b4f2-ff60-7e87-6455c9c10dc8@oracle.com>
Date:   Sun, 27 Nov 2022 08:16:24 +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] vduse: Fix a possible warning in vduse_create_dev()
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     error27@gmail.com, harshit.m.mogalapalli@gmail.com,
        Jason Wang <jasowang@redhat.com>,
        Xie Yongji <xieyongji@bytedance.com>,
        Gautam Dawar <gautam.dawar@xilinx.com>,
        Maxime Coquelin <maxime.coquelin@redhat.com>,
        Guanjun <guanjun@linux.alibaba.com>,
        Parav Pandit <parav@nvidia.com>, Eli Cohen <elic@nvidia.com>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org,
        Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
References: <20221126040000.775914-1-harshit.m.mogalapalli@oracle.com>
 <20221126181822-mutt-send-email-mst@kernel.org>
Content-Language: en-US
From:   Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <20221126181822-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L2dCNnh5b2ZCSFJVeU1ZR3loMm9SN0tYZ2hQRGtpbERKL0h2eUZqV1U2WGs3?=
 =?utf-8?B?cW40S2hOSXFPZzRHcUN0dkRnRnkxbWxROW9UejgxcW4vWEpsSG1qWmUvQ3hh?=
 =?utf-8?B?N3NucElmSjJkcVZSVXIvaWJDOERGTDBYYjRHdG1SUHQzVzczTzhnRXFva2Y1?=
 =?utf-8?B?bEQ1WkVzaTRVdTIwb3pvdXN6cCtYVzNDMjBPTSsyVWtYbjZtNk4waFZ6WlI0?=
 =?utf-8?B?c1NQb3MxMU00eGVXc0xMTWk4SUliYzlJN1ZoMWxXMmNXTVpHUVdnN2JPTmdU?=
 =?utf-8?B?eWJuY2Nla0VyMXpUWW5Xdy9LaStTdXNqSlJEQW1pdklZdS81Qk1kcmcrQkpx?=
 =?utf-8?B?TGlaVFQzaTAzb1FIM1dSUjBaS2d0bVlWai9ud0NMSTdMdlJQUkhaTjhTZ2tM?=
 =?utf-8?B?K2F0SkhzSHEvRHBkbTd1M05kQmtIOHVRSzRNNlg1S28rSUJaVFBRWUlNT3U3?=
 =?utf-8?B?UTFLVDdGa1FKdXNxdHl3TGtLSDdOVnJjREdENWpGN3Yxa1pLb3ErK2pGb2lW?=
 =?utf-8?B?WVdKVmxGd0E1bnBMN2ZJbEE4b3F0SnR1R3dVd2JlUXcyenl3b0FjaUs1NFh0?=
 =?utf-8?B?N3d4aGY3L0x2aVVWclBVcnFvVXlyOGVOcWRGSEZOYWtaNVA2amhVbERoZ0tI?=
 =?utf-8?B?b3htVjk3V29hcXVzRHpjVWQzVzdMQjhlUkkzUEtBci9CeWpsVS9RUldDVlEx?=
 =?utf-8?B?eXI5YkE0TGIwVkUvUmFYKzg2ajRyKzQ1V3NWSTU4c1U3eGZmME1JalhhRnRT?=
 =?utf-8?B?QTBpb1RobVJ3VS9Bd0EvK3EzNVhtQm9tNmN2Y2NHSzlWdUk2V3ZaUXc2cmVB?=
 =?utf-8?B?eHFieTArRW5BK3ZFQUNMT2RiS0NyakpRTlo4V2lHYnNRMk40OFJtYko5ZTNh?=
 =?utf-8?B?QTlLSXNIdmxUc1RpQVRBTlAvVHhES3hXRTN0aUZjVnhpZk0xRmNyRzRDU3NS?=
 =?utf-8?B?OXZjL3YySVRnZHlLQVZPdy9mcmR4YXowQmNvcSs3dEF4M2JYcjgwSjRmMmEy?=
 =?utf-8?B?c1dEQi8vTXQ4ajlrU2hGZEpJemx2VVlPMHBNRys2cDFNYlJkODZNdFh0WUlz?=
 =?utf-8?B?MUdMSGF1MXJwWXUyRHNoOHYwRll3SkpNK1l2WkNjanhwZnNUK1pDbmExYUhi?=
 =?utf-8?B?UTlIRFBDdkdRdy8zajBmdWJDTWVpMFFGMnFLeEx0Q2ZMYjdVbjhKT2tJclVj?=
 =?utf-8?B?WFZpWWF4K0FZejI3anR4eTVrOElQQ3BGR1kxL1ZRbnRlOUVYd285ckYyQ0Np?=
 =?utf-8?B?aVRSMFgwWVNLUktXeWE2cGxtMXJ2MFNJcHJHSWcyeWhuRGZNNjNpN0tvRldm?=
 =?utf-8?B?NUZyKzN4ZmdRakhwZmgvaTl2a1VUY0VSeW9ZbjA5UzNiVlN2bTd5QWVYbWxq?=
 =?utf-8?B?MFNuaHhZLzVFMTgrNmEyR2YxdGJ4UHU2QXhZRW9JWm51ZUF4MndPRFN3SnRS?=
 =?utf-8?B?WTNtcGpvMHoyZnAvRDNiWUpLdVFlaWlkTk9nR0hMQ2dWeG9TbVdWa21RdENU?=
 =?utf-8?B?VXhmT0tiaXJJTmpvNW1vMWE0eHJZYnQ4TTB1ZXZqK2pXdS9BWWZFcGg3MzZ5?=
 =?utf-8?B?d20xdE1KUitwZG55cGtLRlRDNE5KU0QvY2xoZ0FJbjRjMHJzYXc1K0pxOEVE?=
 =?utf-8?B?WDE4RzVkKysxOHpzOHVWdkozOTBUNmkrYURZYzEvSDlnYUJ4WkxVOFRrTitJ?=
 =?utf-8?B?YzFhN2tiM0VQSktwSTBZbG00S0pReDdiNjdJQU1QVDhDQWRWWjdnanVNaVJO?=
 =?utf-8?B?OUZDZWdNUXI5bEwzanVqRldZWWJLMCtaYTFjR0o4V1dHTDY5L2swZUlQZU9W?=
 =?utf-8?B?anMwZUo3T3lSV29qRklXS0RUT2ZBK2VKUEZqMTBjWHh4YWYvTEhYY2tNMUVX?=
 =?utf-8?B?Y2ZCM2U3ZG8wNENaWDdqZlIwdWZOYk01amRyM0Q2b2R0Y2tJZnRlbUthczVN?=
 =?utf-8?B?NTdkV2pOK3ZKS2lGdCtxZXlFeGxFek4vUzhCODZtUjVZNnJGTy9NamxJMFJt?=
 =?utf-8?B?YjBENzlYQXRZWVBqLy9oUUM2NWRkcUxkVk9XdHdHQlVzaUFYVjRVRHQvT1Vx?=
 =?utf-8?B?dkI1aXQ4SUVlVVR3QTlvWngvQnQzQTAvUk52OHdYcmRucEhuNER3MmZUQnNS?=
 =?utf-8?B?aDhOS0JGUmRPM05kRVMrcFVzc2FqTFJ4SWdDcWFvMzZtazloTDdHS1hTckJ4?=
 =?utf-8?Q?zYzuS3JzOurWv5dfkcUyWWw=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?RGhKMnRScmV3Q3hPMXUrSWlCT01yQi9GOTdXNFE3VFZSUFQwc3hsNUJiSVRT?=
 =?utf-8?B?OTFhNFB1OGpzd0M0S3ovd1FiRWxFWTlyYzk4OTZMbzkvSnNnZmU3SHZBN256?=
 =?utf-8?B?cHVRanY2Umd5TFNwRHhNOHFkQ2UrV1YzYzBHQThxRTQvS0t3TzA5VnFXYzdp?=
 =?utf-8?B?ZFgrY0VFWk84OHNxbkxwdVBqMWhTNUNyWDArV3h0M200UkMxVjFMSVhldFVE?=
 =?utf-8?B?emJaM1BRWFF3c2YxZ1M3OWJTdHg3RFY1V3lTL2dVUW1KcmNZam1KWHVWdVI0?=
 =?utf-8?B?cVlzcGt4N3hhdXFOWHNRaXpSejZLQWR1WGdLU0lzc0FOWEN5d2cyN3pCSnh3?=
 =?utf-8?B?Z0tRMmFKaDduOXpFa2VsSU1oZHAvTm8rYThyTGZsd0g0dElaWnRJUjlIRmxH?=
 =?utf-8?B?ZC9VVk9uWXVFTU9MNjlNWUFNcHhjTEJEMTVERnlhUlJ0N0lvODVJbThISjhm?=
 =?utf-8?B?aEVQWXRXL3FpN2Yyc3RBOEJTdnMydkhsMDlad3NVVjh4L0ZkVHJndkh5NTJW?=
 =?utf-8?B?cVpsdnNWc2dHQkZyZEpwVm9RZlBNeFdnQWZzVk5hTXhtTS9MQjlobHovcVVD?=
 =?utf-8?B?THNYVHN0UGhuTEFaalZFUTg1WXBweFUrL240OWNIenEvSmxQeFIwcGViSVhF?=
 =?utf-8?B?OG9kTWthQ25vOHltMkZIa3hGSWZBUmJ1dTRuSGhxdzZWSG1aTFFLLzJDZW1B?=
 =?utf-8?B?dXh0N25jZjNwZFBiVzBLN3d3ZU53NTE1MnZkeEpVVGd1b0xxNjRGdHNjcWhG?=
 =?utf-8?B?WnZsTm10MHJOMkNGTWwrUUZNQVI4RW9jR1FZcUxjclFMWlZ4OENMcS9OUTFO?=
 =?utf-8?B?MGREZDdLUElrYmticEF3cjhoQnMyWDhUWnRyMVdkWWc1N2xMbU1PdGJXY21k?=
 =?utf-8?B?QjBsYWQ5R1pwVi9LWEpSLzZxWi80QldrakoyNVJaVUw3cW5TWmI5MUdGbVpS?=
 =?utf-8?B?bytETThTcHpST2JBSHZFckl5WEExZ2dSU1NsZXJQUTBHL3h3L1AxWFRpejM2?=
 =?utf-8?B?YmduTHZqRG9sWmR3a0pjd2RuY2lUZGNlb3JJWVFTdENpVEg1VVRRbXhucWpm?=
 =?utf-8?B?YWUzanV0ZG5ydEdFaVpOWFpOdDYvdzFMV2hyb1BPMUFIdTFyeGhiQ3JhMFYz?=
 =?utf-8?B?RGsvNHVwN3QrOFFsbHRINHhtRVlraEpHMUdyYkN6Nk80ZG0vb1ljSDFrcTRq?=
 =?utf-8?B?bGlvRUo3NkN0UlJ6aW5rOEpYYVJmMlo2M3ZhZGVQVnVKWFpZSjB5TDdRbUsr?=
 =?utf-8?B?L2FQQzR2TVo4aXM5QjFEUWJDem9yNmk0NmJ1SzhwSmcvZ2xrL2dINzI1TEMw?=
 =?utf-8?B?SnZqMWE5VkE1cCtFK1QvckpHQitrZ1p5VUxvT3ozNTBaKzlIMnVQSnhoZkox?=
 =?utf-8?B?QW8rNVYrRXgzWGpLd3A0dG9rd2VXek1mVnZhUTZUb0FncUNBc3MrS1BKU0Rh?=
 =?utf-8?B?aUFVb3Q3VWpCd3A3YThuWDlVNHY2dWhndG9sZzU3MU83YzJFTDVjbjJPdElD?=
 =?utf-8?B?RG9PL3g5WjdKWnNHek50cnZLOC9OL1p4MkpOT3o0UXF2K2pIaDEzemN6UHZn?=
 =?utf-8?B?WnpjUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 792283de-44a3-47ee-6ee1-08dad021996c
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6290.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Nov 2022 02:46:35.4270
 (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: QIe318GPRQtXcJItBjOBsU6/RjpFTD9W7O46EE981D3GgIJBjp+bUun+0oPDDzR5mugJwKkix32dlgfzHhuwebl35n07m3ZXDNH0ueZ7Y9LNjDbhZthH+aJNWQUmkQGv
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5024
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-26_16,2022-11-25_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 phishscore=0
 mlxlogscore=999 mlxscore=0 adultscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211270019
X-Proofpoint-GUID: T21hEky8ynlrjwdWzQkM6zithMqCLurs
X-Proofpoint-ORIG-GUID: T21hEky8ynlrjwdWzQkM6zithMqCLurs
X-Spam-Status: No, score=-2.8 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

Hi Micheal,

On 27/11/22 4:52 am, Michael S. Tsirkin wrote:
> On Fri, Nov 25, 2022 at 07:59:58PM -0800, Harshit Mogalapalli wrote:
>> As 'dev->vq_num' is user-controlled data, if user tries to allocate
>> memory larger than(>=) MAX_ORDER, then kcalloc() will fail, it
>> creates a stack trace and messes up dmesg with a warning.
>>
>> Call trace:
>> -> vduse_ioctl
>> --> vduse_create_dev
>> 'config->vq_num' is user data as it comes from ioctl, which is
>> assigned to 'dev->vq_num'.
>>
>> Add __GFP_NOWARN in order to avoid too large allocation warning.
>> This is detected by static analysis using smatch.
>>
>> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>>   drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 35dceee3ed56..5e9546b16165 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1512,7 +1512,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>>   	dev->config_size = config->config_size;
>>   	dev->vq_align = config->vq_align;
>>   	dev->vq_num = config->vq_num;
>> -	dev->vqs = kcalloc(dev->vq_num, sizeof(*dev->vqs), GFP_KERNEL);
>> +	dev->vqs = kcalloc(dev->vq_num, sizeof(*dev->vqs),
>> +			   GFP_KERNEL | __GFP_NOWARN);
>>   	if (!dev->vqs)
>>   		goto err_vqs;
> 

Thanks for checking the patch.

> This is insufficient - the real source of the problem is that
> vq_num is not validated.
> The thing to do is to validate config and limit vq_num to 0xffff;
> 

1557 static long vduse_ioctl(struct file *file, unsigned int cmd,
1558                         unsigned long arg)
1559 {
1560         int ret;
1561         void __user *argp = (void __user *)arg;
1564         mutex_lock(&vduse_lock);
1565         switch (cmd) {
              ....
1584         case VDUSE_CREATE_DEV: {
1585                 struct vduse_dev_config config;
1587                 void *buf;
1588
1589                 ret = -EFAULT;
1590                 if (copy_from_user(&config, argp, size))
1591                         break;
1592
1593                 ret = -EINVAL;
1594                 if (vduse_validate_config(&config) == false)
1595                         break;
1596
1597                 buf = vmemdup_user(argp + size, config.config_size);
1598                 if (IS_ERR(buf)) {
1599                         ret = PTR_ERR(buf);
1600                         break;
1601                 }
1602                 config.name[VDUSE_NAME_MAX - 1] = '\0';
1603                 ret = vduse_create_dev(&config, buf, 
control->api_version);
1604                 if (ret)
1605                         kvfree(buf);
1606                 break;
1607         }

we have vduse_validate_config() being called in vduse_ioctl() which is 
the caller of vduse_create_dev(), so validate_config() is not necessary 
in vduse_create_dev() ?

Thanks,
Harshit

> 
>> -- 
>> 2.38.1
>