* [bug report] iommufd: vfio container FD ioctl compatibility
@ 2022-11-15 12:45 Dan Carpenter
2022-11-15 20:29 ` Jason Gunthorpe
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-11-15 12:45 UTC (permalink / raw)
To: jgg; +Cc: iommu
[ Resending two weeks of email because mutt + msmtp has been silently
eating my outgoing mail instead of delivering it. *sigh* -dan ]
Hello Jason Gunthorpe,
The patch 32c328dc9b73: "iommufd: vfio container FD ioctl
compatibility" from Dec 15, 2021, leads to the following Smatch
static checker warning:
drivers/iommu/iommufd/vfio_compat.c:174 iommufd_vfio_unmap_dma()
warn: potential integer overflow from user (local copy) 'unmap.iova + unmap.size'
drivers/iommu/iommufd/vfio_compat.c
140 static int iommufd_vfio_unmap_dma(struct iommufd_ctx *ictx, unsigned int cmd,
141 void __user *arg)
142 {
143 size_t minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
144 /*
145 * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP is obsoleted by the new
146 * dirty tracking direction:
147 * https://lore.kernel.org/kvm/20220731125503.142683-1-yishaih@nvidia.com/
148 * https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@oracle.com/
149 */
150 u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL;
151 struct vfio_iommu_type1_dma_unmap unmap;
152 struct iommufd_ioas *ioas;
153 unsigned long unmapped;
154 int rc;
155
156 if (copy_from_user(&unmap, arg, minsz))
157 return -EFAULT;
158
159 if (unmap.argsz < minsz || unmap.flags & ~supported_flags)
160 return -EINVAL;
161
162 ioas = get_compat_ioas(ictx);
163 if (IS_ERR(ioas))
164 return PTR_ERR(ioas);
165
166 if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL) {
167 if (unmap.iova != 0 || unmap.size != 0) {
168 rc = -EINVAL;
169 goto err_put;
170 }
171 rc = iopt_unmap_all(&ioas->iopt, &unmapped);
172 } else {
173 if (READ_ONCE(ioas->iopt.disable_large_pages)) {
--> 174 unsigned long iovas[] = { unmap.iova + unmap.size - 1,
The unmap.iova + unmap.size addition can have an integer overflow. It's
unclear to me if this is caught later or if it has any negative
implications.
What I remeber from looking at similar code is that because of the - 1,
it's supposed to be allowed to overflow to 0 but not further than that.
In other words the highest allowed value it ULONG_MAX and not
"ULONG_MAX - 1".
175 unmap.iova - 1 };
176
177 rc = iopt_cut_iova(&ioas->iopt, iovas,
178 unmap.iova ? 2 : 1);
179 if (rc)
180 goto err_put;
181 }
182 rc = iopt_unmap_iova(&ioas->iopt, unmap.iova, unmap.size,
183 &unmapped);
184 }
185 unmap.size = unmapped;
186 if (copy_to_user(arg, &unmap, minsz))
187 rc = -EFAULT;
188
189 err_put:
190 iommufd_put_object(&ioas->obj);
191 return rc;
192 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] iommufd: vfio container FD ioctl compatibility
2022-11-15 12:45 [bug report] iommufd: vfio container FD ioctl compatibility Dan Carpenter
@ 2022-11-15 20:29 ` Jason Gunthorpe
2022-11-16 18:11 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Jason Gunthorpe @ 2022-11-15 20:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: iommu
On Tue, Nov 15, 2022 at 03:45:00PM +0300, Dan Carpenter wrote:
> [ Resending two weeks of email because mutt + msmtp has been silently
> eating my outgoing mail instead of delivering it. *sigh* -dan ]
>
> Hello Jason Gunthorpe,
>
> The patch 32c328dc9b73: "iommufd: vfio container FD ioctl
> compatibility" from Dec 15, 2021, leads to the following Smatch
> static checker warning:
>
> drivers/iommu/iommufd/vfio_compat.c:174 iommufd_vfio_unmap_dma()
> warn: potential integer overflow from user (local copy) 'unmap.iova + unmap.size'
>
> drivers/iommu/iommufd/vfio_compat.c
> 140 static int iommufd_vfio_unmap_dma(struct iommufd_ctx *ictx, unsigned int cmd,
> 141 void __user *arg)
> 142 {
> 143 size_t minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
> 144 /*
> 145 * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP is obsoleted by the new
> 146 * dirty tracking direction:
> 147 * https://lore.kernel.org/kvm/20220731125503.142683-1-yishaih@nvidia.com/
> 148 * https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@oracle.com/
> 149 */
> 150 u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL;
> 151 struct vfio_iommu_type1_dma_unmap unmap;
> 152 struct iommufd_ioas *ioas;
> 153 unsigned long unmapped;
> 154 int rc;
> 155
> 156 if (copy_from_user(&unmap, arg, minsz))
> 157 return -EFAULT;
> 158
> 159 if (unmap.argsz < minsz || unmap.flags & ~supported_flags)
> 160 return -EINVAL;
> 161
> 162 ioas = get_compat_ioas(ictx);
> 163 if (IS_ERR(ioas))
> 164 return PTR_ERR(ioas);
> 165
> 166 if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL) {
> 167 if (unmap.iova != 0 || unmap.size != 0) {
> 168 rc = -EINVAL;
> 169 goto err_put;
> 170 }
> 171 rc = iopt_unmap_all(&ioas->iopt, &unmapped);
> 172 } else {
> 173 if (READ_ONCE(ioas->iopt.disable_large_pages)) {
> --> 174 unsigned long iovas[] = { unmap.iova + unmap.size - 1,
>
> The unmap.iova + unmap.size addition can have an integer overflow. It's
> unclear to me if this is caught later or if it has any negative
> implications.
There are no negative implications to the kernel, which is why I left
it unchecked here.
Do you think we should have the check for the sake of static tools?
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] iommufd: vfio container FD ioctl compatibility
2022-11-15 20:29 ` Jason Gunthorpe
@ 2022-11-16 18:11 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-11-16 18:11 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu
On Tue, Nov 15, 2022 at 04:29:20PM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 15, 2022 at 03:45:00PM +0300, Dan Carpenter wrote:
> > [ Resending two weeks of email because mutt + msmtp has been silently
> > eating my outgoing mail instead of delivering it. *sigh* -dan ]
> >
> > Hello Jason Gunthorpe,
> >
> > The patch 32c328dc9b73: "iommufd: vfio container FD ioctl
> > compatibility" from Dec 15, 2021, leads to the following Smatch
> > static checker warning:
> >
> > drivers/iommu/iommufd/vfio_compat.c:174 iommufd_vfio_unmap_dma()
> > warn: potential integer overflow from user (local copy) 'unmap.iova + unmap.size'
> >
> > drivers/iommu/iommufd/vfio_compat.c
> > 140 static int iommufd_vfio_unmap_dma(struct iommufd_ctx *ictx, unsigned int cmd,
> > 141 void __user *arg)
> > 142 {
> > 143 size_t minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
> > 144 /*
> > 145 * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP is obsoleted by the new
> > 146 * dirty tracking direction:
> > 147 * https://lore.kernel.org/kvm/20220731125503.142683-1-yishaih@nvidia.com/
> > 148 * https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@oracle.com/
> > 149 */
> > 150 u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL;
> > 151 struct vfio_iommu_type1_dma_unmap unmap;
> > 152 struct iommufd_ioas *ioas;
> > 153 unsigned long unmapped;
> > 154 int rc;
> > 155
> > 156 if (copy_from_user(&unmap, arg, minsz))
> > 157 return -EFAULT;
> > 158
> > 159 if (unmap.argsz < minsz || unmap.flags & ~supported_flags)
> > 160 return -EINVAL;
> > 161
> > 162 ioas = get_compat_ioas(ictx);
> > 163 if (IS_ERR(ioas))
> > 164 return PTR_ERR(ioas);
> > 165
> > 166 if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL) {
> > 167 if (unmap.iova != 0 || unmap.size != 0) {
> > 168 rc = -EINVAL;
> > 169 goto err_put;
> > 170 }
> > 171 rc = iopt_unmap_all(&ioas->iopt, &unmapped);
> > 172 } else {
> > 173 if (READ_ONCE(ioas->iopt.disable_large_pages)) {
> > --> 174 unsigned long iovas[] = { unmap.iova + unmap.size - 1,
> >
> > The unmap.iova + unmap.size addition can have an integer overflow. It's
> > unclear to me if this is caught later or if it has any negative
> > implications.
>
> There are no negative implications to the kernel, which is why I left
> it unchecked here.
>
> Do you think we should have the check for the sake of static tools?
No. I've written something like five different integer overflow checks.
One is pretty decent but the problem is a hard nut to crack. This
heuristic here is not useful enough to the point where we'd want to
silence false positives.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-16 18:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-15 12:45 [bug report] iommufd: vfio container FD ioctl compatibility Dan Carpenter
2022-11-15 20:29 ` Jason Gunthorpe
2022-11-16 18:11 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox