[bug report] iommu/vt-d: Fix kdump kernels boot failure with scalable mode

[bug report] iommu/vt-d: Fix kdump kernels boot failure with scalable mode

[Dan Carpenter]

Hello Lu Baolu,

The patch 0c5f6c0d8201: "iommu/vt-d: Fix kdump kernels boot failure
with scalable mode" from Aug 23, 2022, leads to the following Smatch
static checker warning:

drivers/iommu/intel/iommu.c:224 set_context_copied() warn: set_bit() takes a bit number
drivers/iommu/intel/iommu.c:230 clear_context_copied() warn: clear_bit() takes a bit number

drivers/iommu/intel/iommu.c
    221 static inline void
    222 set_context_copied(struct intel_iommu *iommu, u8 bus, u8 devfn)
    223 {
--> 224         set_bit(((long)bus << 8) | devfn, iommu->copied_tables);
    225 }

This is trying to set a mask but it will instead corrupt a bit way out
in the middle of your memory.  The set_bit function will only set one
bit at a time.  If we want to set bit zero:

	set_bit(0, iommu->copied_tables);

Or if we have a whole page full of bits then we could set the last one:

	set_bit(32767, page);

regards,
dan carpenter

Re: [bug report] iommu/vt-d: Fix kdump kernels boot failure with scalable mode

[Robin Murphy]

On 2022-09-15 11:05, Dan Carpenter wrote:
> Hello Lu Baolu,
> 
> The patch 0c5f6c0d8201: "iommu/vt-d: Fix kdump kernels boot failure
> with scalable mode" from Aug 23, 2022, leads to the following Smatch
> static checker warning:
> 
> drivers/iommu/intel/iommu.c:224 set_context_copied() warn: set_bit() takes a bit number
> drivers/iommu/intel/iommu.c:230 clear_context_copied() warn: clear_bit() takes a bit number
> 
> drivers/iommu/intel/iommu.c
>      221 static inline void
>      222 set_context_copied(struct intel_iommu *iommu, u8 bus, u8 devfn)
>      223 {
> --> 224         set_bit(((long)bus << 8) | devfn, iommu->copied_tables);
>      225 }
> 
> This is trying to set a mask

No, it's simply composing a full 16-bit PCI requester ID from its two 
8-bit components.

> but it will instead corrupt a bit way out
> in the middle of your memory.

	iommu->copied_tables = bitmap_zalloc(BIT_ULL(16), GFP_KERNEL);

Again slightly non-obvious, but AFAICS the bitmap is sized appropriately.

>  The set_bit function will only set one
> bit at a time.  If we want to set bit zero:
> 
> 	set_bit(0, iommu->copied_tables);
> 
> Or if we have a whole page full of bits then we could set the last one:
> 
> 	set_bit(32767, page);

I believe that's exactly what we're doing here, setting one bit for each 
PCI ID processed. It's just a very big bitmap thanks to the size of the 
PCI ID space.

Cheers,
Robin.

Re: [bug report] iommu/vt-d: Fix kdump kernels boot failure with scalable mode

[Dan Carpenter]

On Thu, Sep 15, 2022 at 11:21:36AM +0100, Robin Murphy wrote:
> On 2022-09-15 11:05, Dan Carpenter wrote:
> > Hello Lu Baolu,
> > 
> > The patch 0c5f6c0d8201: "iommu/vt-d: Fix kdump kernels boot failure
> > with scalable mode" from Aug 23, 2022, leads to the following Smatch
> > static checker warning:
> > 
> > drivers/iommu/intel/iommu.c:224 set_context_copied() warn: set_bit() takes a bit number
> > drivers/iommu/intel/iommu.c:230 clear_context_copied() warn: clear_bit() takes a bit number
> > 
> > drivers/iommu/intel/iommu.c
> >      221 static inline void
> >      222 set_context_copied(struct intel_iommu *iommu, u8 bus, u8 devfn)
> >      223 {
> > --> 224         set_bit(((long)bus << 8) | devfn, iommu->copied_tables);
> >      225 }
> > 
> > This is trying to set a mask
> 
> No, it's simply composing a full 16-bit PCI requester ID from its two 8-bit
> components.
> 

Ah...  Okay.  That works then.

> > but it will instead corrupt a bit way out
> > in the middle of your memory.
> 
> 	iommu->copied_tables = bitmap_zalloc(BIT_ULL(16), GFP_KERNEL);
> 
> Again slightly non-obvious, but AFAICS the bitmap is sized appropriately.

Btw, just BIT(16) works until you get above 31.

regards,
dan carpenter