On Wed, Dec 09, 2020 at 10:01:49AM +0000, Song Bao Hua (Barry Song) wrote: > > > > -----Original Message----- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Wednesday, December 9, 2020 8:00 PM > > To: Song Bao Hua (Barry Song) <song.bao....@hisilicon.com> > > Cc: iommu@lists.linux-foundation.org > > Subject: [bug report] dma-mapping: add benchmark support for streaming DMA > > APIs > > > > Hello Barry Song, > > > > The patch 65789daa8087: "dma-mapping: add benchmark support for > > streaming DMA APIs" from Nov 16, 2020, leads to the following static > > checker warning: > > > > kernel/dma/map_benchmark.c:241 map_benchmark_ioctl() > > error: undefined (user controlled) shift '1 << (map->bparam.dma_bits)' > > > > kernel/dma/map_benchmark.c > > 191 static long map_benchmark_ioctl(struct file *file, unsigned int cmd, > > 192 unsigned long arg) > > 193 { > > 194 struct map_benchmark_data *map = file->private_data; > > 195 void __user *argp = (void __user *)arg; > > 196 u64 old_dma_mask; > > 197 > > 198 int ret; > > 199 > > 200 if (copy_from_user(&map->bparam, argp, sizeof(map->bparam))) > > ^^^^^^^^^^^^^ > > Comes from the user > > > > 201 return -EFAULT; > > 202 > > 203 switch (cmd) { > > 204 case DMA_MAP_BENCHMARK: > > 205 if (map->bparam.threads == 0 || > > 206 map->bparam.threads > DMA_MAP_MAX_THREADS) { > > 207 pr_err("invalid thread number\n"); > > 208 return -EINVAL; > > 209 } > > 210 > > 211 if (map->bparam.seconds == 0 || > > 212 map->bparam.seconds > DMA_MAP_MAX_SECONDS) { > > 213 pr_err("invalid duration seconds\n"); > > 214 return -EINVAL; > > 215 } > > 216 > > 217 if (map->bparam.node != NUMA_NO_NODE && > > 218 !node_possible(map->bparam.node)) { > > 219 pr_err("invalid numa node\n"); > > 220 return -EINVAL; > > 221 } > > 222 > > 223 switch (map->bparam.dma_dir) { > > 224 case DMA_MAP_BIDIRECTIONAL: > > 225 map->dir = DMA_BIDIRECTIONAL; > > 226 break; > > 227 case DMA_MAP_FROM_DEVICE: > > 228 map->dir = DMA_FROM_DEVICE; > > 229 break; > > 230 case DMA_MAP_TO_DEVICE: > > 231 map->dir = DMA_TO_DEVICE; > > 232 break; > > 233 default: > > 234 pr_err("invalid DMA direction\n"); > > 235 return -EINVAL; > > 236 } > > 237 > > 238 old_dma_mask = dma_get_mask(map->dev); > > 239 > > 240 ret = dma_set_mask(map->dev, > > 241 > > DMA_BIT_MASK(map->bparam.dma_bits)); > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > If this is more than 31 then the behavior is undefined (but in real life > > it will shift wrap). > > Guess it should be less than 64? > For 64, it would be ~0ULL, otherwise, it will be 1ULL<<n-1
Yeah. You're right > 64 is undefined, not 31 as I said. > > In test app, > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=7679325702 > > > I have some code like: > + /* suppose the mininum DMA zone is 1MB in the world */ > + if (bits < 20 || bits > 64) { > + fprintf(stderr, "invalid dma mask bit, must be in 20-64\n"); > + exit(1); > + } > > Maybe I should do the same thing in kernel as well. Sounds good! regards, dan carpenter _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
