> Subject: [bug report] fpga: dfl: fme: align PR buffer size per PR datawidth
>
> Hello Wu Hao,
>
> The patch 69416739ee36: "fpga: dfl: fme: align PR buffer size per PR
> datawidth" from Jun 27, 2019, leads to the following Smatch static
> checker warning:
>
> drivers/fpga/dfl-fme-pr.c:104 fme_pr()
> warn: potential integer overflow from user '((port_pr.buffer_size)) + (((4))
> - 1)' [local copy]
>
Thanks for reporting this, will add checking for it.
Hao
> drivers/fpga/dfl-fme-pr.c
> 66 static int fme_pr(struct platform_device *pdev, unsigned long arg)
> 67 {
> 68 struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> 69 void __user *argp = (void __user *)arg;
> 70 struct dfl_fpga_fme_port_pr port_pr;
> 71 struct fpga_image_info *info;
> 72 struct fpga_region *region;
> 73 void __iomem *fme_hdr;
> 74 struct dfl_fme *fme;
> 75 unsigned long minsz;
> 76 void *buf = NULL;
> 77 size_t length;
> 78 int ret = 0;
> 79 u64 v;
> 80
> 81 minsz = offsetofend(struct dfl_fpga_fme_port_pr, buffer_address);
> 82
> 83 if (copy_from_user(&port_pr, argp, minsz))
> 84 return -EFAULT;
> 85
> 86 if (port_pr.argsz < minsz || port_pr.flags)
> 87 return -EINVAL;
> 88
> 89 /* get fme header region */
> 90 fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
> 91 FME_FEATURE_ID_HEADER);
> 92
> 93 /* check port id */
> 94 v = readq(fme_hdr + FME_HDR_CAP);
> 95 if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) {
> 96 dev_dbg(&pdev->dev, "port number more than maximum\n");
> 97 return -EINVAL;
> 98 }
> 99
> 100 /*
> 101 * align PR buffer per PR bandwidth, as HW ignores the extra padding
> 102 * data automatically.
> 103 */
> --> 104 length = ALIGN(port_pr.buffer_size, 4);
>
> The ALIGN() macro can wrap to zero if it's >= UINT_MAX - 3.
>
> 105
> 106 buf = vmalloc(length);
>
> It's sort of harmless-ish because vmalloc() will WARN_ON_ONCE() if you
> pass it a zero and return NULL. But we could try to prevent the stack
> trace.
>
> 107 if (!buf)
> 108 return -ENOMEM;
> 109
> 110 if (copy_from_user(buf,
> 111 (void __user *)(unsigned long)port_pr.buffer_address,
> 112 port_pr.buffer_size)) {
>
> This doesn't zero out the padding, so it might be flagged as an info
> leak depending on who is reviewing.
>
> 113 ret = -EFAULT;
> 114 goto free_exit;
> 115 }
> 116
> 117 /* prepare fpga_image_info for PR */
> 118 info = fpga_image_info_alloc(&pdev->dev);
> 119 if (!info) {
> 120 ret = -ENOMEM;
> 121 goto free_exit;
> 122 }
> 123
> 124 info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> 125
> 126 mutex_lock(&pdata->lock);
> 127 fme = dfl_fpga_pdata_get_private(pdata);
> 128 /* fme device has been unregistered. */
> 129 if (!fme) {
> 130 ret = -EINVAL;
> 131 goto unlock_exit;
> 132 }
> 133
> 134 region = dfl_fme_region_find(fme, port_pr.port_id);
> 135 if (!region) {
> 136 ret = -EINVAL;
> 137 goto unlock_exit;
> 138 }
> 139
> 140 fpga_image_info_free(region->info);
> 141
> 142 info->buf = buf;
> 143 info->count = length;
> 144 info->region_id = port_pr.port_id;
> 145 region->info = info;
> 146
> 147 ret = fpga_region_program_fpga(region);
> 148
> 149 /*
> 150 * it allows userspace to reset the PR region's logic by disabling and
> 151 * reenabling the bridge to clear things out between acceleration runs.
> 152 * so no need to hold the bridges after partial reconfiguration.
> 153 */
> 154 if (region->get_bridges)
> 155 fpga_bridges_put(®ion->bridge_list);
> 156
> 157 put_device(®ion->dev);
> 158 unlock_exit:
> 159 mutex_unlock(&pdata->lock);
> 160 free_exit:
> 161 vfree(buf);
> 162 return ret;
> 163 }
>
> regards,
> dan carpenter
