Hi,
On 10/19/22 10:38 PM, Greg Kroah-Hartman wrote:
> On Wed, Oct 19, 2022 at 09:58:27PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> +static long tdx_get_report(void __user *argp)
>> +{
>> + u8 *reportdata, *tdreport;
>> + struct tdx_report_req req;
>> + long ret;
>> +
>> + if (copy_from_user(&req, argp, sizeof(req)))
>> + return -EFAULT;
>> +
>> + /*
>> + * Per TDX Module 1.0 specification, section titled
>> + * "TDG.MR.REPORT", REPORTDATA length is fixed as
>> + * TDX_REPORTDATA_LEN, TDREPORT length is fixed as
>> + * TDX_REPORT_LEN, and TDREPORT subtype is fixed as 0.
>> + */
>> + if (req.subtype || req.rpd_len != TDX_REPORTDATA_LEN ||
>> + req.tdr_len != TDX_REPORT_LEN) {
>> + pr_err("TDX_CMD_GET_REPORT: invalid req: subtype:%u rpd_len:%u tdr_len:%u\n",
>> + req.subtype, req.rpd_len, req.tdr_len);
>
> You are allowing userspace to spam the kernel logs, please do not do
> that.
Added it to help userspace understand the reason for the failure (only for
the cases like request param issues and TDCALL failure). Boris recommended
adding it in the previous review.
>
> Also, you have a real device here, use it and call dev_*() instead of
> pr_*(). Your code should not have any pr_* calls.
Ok. I will use dev_err variant.
>
>
>> + return -EINVAL;
>> + }
>> +
>> + if (memchr_inv(req.reserved, 0, sizeof(req.reserved))) {
>> + pr_err("TDX_CMD_GET_REPORT: Non zero value in reserved field\n");
>> + return -EINVAL;
>> + }
>> +
>> + reportdata = kmalloc(req.rpd_len, GFP_KERNEL);
>> + if (!reportdata)
>> + return -ENOMEM;
>> +
>> + tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
>> + if (!tdreport) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + if (copy_from_user(reportdata, u64_to_user_ptr(req.reportdata),
>> + req.rpd_len)) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + /* Generate TDREPORT using "TDG.MR.REPORT" TDCALL */
>> + ret = tdx_mcall_get_report(reportdata, tdreport, req.subtype);
>> + if (ret) {
>> + pr_err("TDX_CMD_GET_REPORT: TDCALL failed\n");
>> + goto out;
>> + }
>> +
>> + if (copy_to_user(u64_to_user_ptr(req.tdreport), tdreport, req.tdr_len))
>> + ret = -EFAULT;
>> +
>> +out:
>> + kfree(reportdata);
>> + kfree(tdreport);
>> +
>> + return ret;
>> +}
>> +
>> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + switch (cmd) {
>> + case TDX_CMD_GET_REPORT:
>> + return tdx_get_report((void __user *)arg);
>> + default:
>> + return -ENOTTY;
>> + }
>> +}
>> +
>> +static const struct file_operations tdx_guest_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = tdx_guest_ioctl,
>> + .llseek = no_llseek,
>> +};
>> +
>> +static struct miscdevice tdx_misc_dev = {
>> + .name = KBUILD_MODNAME,
>> + .minor = MISC_DYNAMIC_MINOR,
>> + .fops = &tdx_guest_fops,
>> +};
>> +
>> +static int __init tdx_guest_init(void)
>> +{
>> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> + return -ENODEV;
>> +
>> + return misc_register(&tdx_misc_dev);
>> +}
>> +module_init(tdx_guest_init);
>> +
>> +static void __exit tdx_guest_exit(void)
>> +{
>> + misc_deregister(&tdx_misc_dev);
>> +}
>> +module_exit(tdx_guest_exit);
>> +
>> +#ifdef MODULE
>> +static const struct x86_cpu_id tdx_guest_ids[] = {
>> + X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>> +#endif
>
> Why the #ifdef? Should not be needed, right?
I have added it to fix the following warning reported by 0-day.
https://lore.kernel.org/lkml/202209211607.tCtTWKbV-lkp@xxxxxxxxx/
It is related to nullifying the MODULE_DEVICE_TABLE in #ifndef MODULE
case in linux/module.h.
>
> thanks,
>
> greg k-h
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
