| Message ID | 6211be8a-5d10-8f3a-6d33-af695dc35caf@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/VPD: Fix stack overflow caused by pci_read_vpd_any() | expand |
On Wed, Oct 13, 2021 at 08:19:59PM +0200, Heiner Kallweit wrote: > Recent bug fix 00e1a5d21b4f ("PCI/VPD: Defer VPD sizing until first > access") interferes with the original change, resulting in a stack > overflow. The following fix has been successfully tested by Qian > and myself. What does "the original change" refer to? 80484b7f8db1? I guess the stack overflow is an unintended recursion? Is there a URL to Qian's bug report with more details that we can include here? > Fixes: 80484b7f8db1 ("PCI/VPD: Use pci_read_vpd_any() in pci_vpd_size()") > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > Tested-by: Qian Cai <quic_qiancai@quicinc.com> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/pci/vpd.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 5108bbd20..a4fc4d069 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -96,14 +96,14 @@ static size_t pci_vpd_size(struct pci_dev *dev) > return off ?: PCI_VPD_SZ_INVALID; > } > > -static bool pci_vpd_available(struct pci_dev *dev) > +static bool pci_vpd_available(struct pci_dev *dev, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > > if (!vpd->cap) > return false; > > - if (vpd->len == 0) { > + if (vpd->len == 0 && check_size) { > vpd->len = pci_vpd_size(dev); > if (vpd->len == PCI_VPD_SZ_INVALID) { > vpd->cap = 0; > @@ -156,17 +156,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > void *arg, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + unsigned int max_len; > int ret = 0; > loff_t end = pos + count; > u8 *buf = arg; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, check_size)) > return -ENODEV; > > if (pos < 0) > return -EINVAL; > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + > if (pos >= max_len) > return 0; > > @@ -218,17 +220,19 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > const void *arg, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + unsigned int max_len; > const u8 *buf = arg; > loff_t end = pos + count; > int ret = 0; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, check_size)) > return -ENODEV; > > if (pos < 0 || (pos & 3) || (count & 3)) > return -EINVAL; > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + > if (end > max_len) > return -EINVAL; > > @@ -312,7 +316,7 @@ void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size) > void *buf; > int cnt; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, true)) > return ERR_PTR(-ENODEV); > > len = dev->vpd.len; > -- > 2.33.0 >
On 13.10.2021 20:53, Bjorn Helgaas wrote: > On Wed, Oct 13, 2021 at 08:19:59PM +0200, Heiner Kallweit wrote: >> Recent bug fix 00e1a5d21b4f ("PCI/VPD: Defer VPD sizing until first >> access") interferes with the original change, resulting in a stack >> overflow. The following fix has been successfully tested by Qian >> and myself. > > What does "the original change" refer to? 80484b7f8db1? I guess the > stack overflow is an unintended recursion? Is there a URL to Qian's > bug report with more details that we can include here? > 1. yes 2. yes 3. https://lore.kernel.org/netdev/e89087c5-c495-c5ca-feb1-54cf3a8775c5@quicinc.com/ >> Fixes: 80484b7f8db1 ("PCI/VPD: Use pci_read_vpd_any() in pci_vpd_size()") >> Reported-by: Qian Cai <quic_qiancai@quicinc.com> >> Tested-by: Qian Cai <quic_qiancai@quicinc.com> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/pci/vpd.c | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >> index 5108bbd20..a4fc4d069 100644 >> --- a/drivers/pci/vpd.c >> +++ b/drivers/pci/vpd.c >> @@ -96,14 +96,14 @@ static size_t pci_vpd_size(struct pci_dev *dev) >> return off ?: PCI_VPD_SZ_INVALID; >> } >> >> -static bool pci_vpd_available(struct pci_dev *dev) >> +static bool pci_vpd_available(struct pci_dev *dev, bool check_size) >> { >> struct pci_vpd *vpd = &dev->vpd; >> >> if (!vpd->cap) >> return false; >> >> - if (vpd->len == 0) { >> + if (vpd->len == 0 && check_size) { >> vpd->len = pci_vpd_size(dev); >> if (vpd->len == PCI_VPD_SZ_INVALID) { >> vpd->cap = 0; >> @@ -156,17 +156,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, >> void *arg, bool check_size) >> { >> struct pci_vpd *vpd = &dev->vpd; >> - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; >> + unsigned int max_len; >> int ret = 0; >> loff_t end = pos + count; >> u8 *buf = arg; >> >> - if (!pci_vpd_available(dev)) >> + if (!pci_vpd_available(dev, check_size)) >> return -ENODEV; >> >> if (pos < 0) >> return -EINVAL; >> >> + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; >> + >> if (pos >= max_len) >> return 0; >> >> @@ -218,17 +220,19 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> const void *arg, bool check_size) >> { >> struct pci_vpd *vpd = &dev->vpd; >> - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; >> + unsigned int max_len; >> const u8 *buf = arg; >> loff_t end = pos + count; >> int ret = 0; >> >> - if (!pci_vpd_available(dev)) >> + if (!pci_vpd_available(dev, check_size)) >> return -ENODEV; >> >> if (pos < 0 || (pos & 3) || (count & 3)) >> return -EINVAL; >> >> + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; >> + >> if (end > max_len) >> return -EINVAL; >> >> @@ -312,7 +316,7 @@ void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size) >> void *buf; >> int cnt; >> >> - if (!pci_vpd_available(dev)) >> + if (!pci_vpd_available(dev, true)) >> return ERR_PTR(-ENODEV); >> >> len = dev->vpd.len; >> -- >> 2.33.0 >>
On Wed, Oct 13, 2021 at 08:19:59PM +0200, Heiner Kallweit wrote: > Recent bug fix 00e1a5d21b4f ("PCI/VPD: Defer VPD sizing until first > access") interferes with the original change, resulting in a stack > overflow. The following fix has been successfully tested by Qian > and myself. > > Fixes: 80484b7f8db1 ("PCI/VPD: Use pci_read_vpd_any() in pci_vpd_size()") > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > Tested-by: Qian Cai <quic_qiancai@quicinc.com> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> What does this apply to? > --- > drivers/pci/vpd.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 5108bbd20..a4fc4d069 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -96,14 +96,14 @@ static size_t pci_vpd_size(struct pci_dev *dev) > return off ?: PCI_VPD_SZ_INVALID; > } > > -static bool pci_vpd_available(struct pci_dev *dev) > +static bool pci_vpd_available(struct pci_dev *dev, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > > if (!vpd->cap) > return false; > > - if (vpd->len == 0) { > + if (vpd->len == 0 && check_size) { > vpd->len = pci_vpd_size(dev); > if (vpd->len == PCI_VPD_SZ_INVALID) { > vpd->cap = 0; > @@ -156,17 +156,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > void *arg, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + unsigned int max_len; > int ret = 0; > loff_t end = pos + count; > u8 *buf = arg; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, check_size)) > return -ENODEV; > > if (pos < 0) > return -EINVAL; > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + > if (pos >= max_len) > return 0; > > @@ -218,17 +220,19 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > const void *arg, bool check_size) > { > struct pci_vpd *vpd = &dev->vpd; > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + unsigned int max_len; > const u8 *buf = arg; > loff_t end = pos + count; > int ret = 0; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, check_size)) > return -ENODEV; > > if (pos < 0 || (pos & 3) || (count & 3)) > return -EINVAL; > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > + > if (end > max_len) > return -EINVAL; > > @@ -312,7 +316,7 @@ void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size) > void *buf; > int cnt; > > - if (!pci_vpd_available(dev)) > + if (!pci_vpd_available(dev, true)) > return ERR_PTR(-ENODEV); > > len = dev->vpd.len; > -- > 2.33.0 >
On Mon, Oct 25, 2021 at 03:57:00PM -0500, Bjorn Helgaas wrote: > On Wed, Oct 13, 2021 at 08:19:59PM +0200, Heiner Kallweit wrote: > > Recent bug fix 00e1a5d21b4f ("PCI/VPD: Defer VPD sizing until first > > access") interferes with the original change, resulting in a stack > > overflow. The following fix has been successfully tested by Qian > > and myself. > > > > Fixes: 80484b7f8db1 ("PCI/VPD: Use pci_read_vpd_any() in pci_vpd_size()") > > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > > Tested-by: Qian Cai <quic_qiancai@quicinc.com> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > What does this apply to? Never mind, I'm an idiot. Obviously this fixes 80484b7f8db1 which is a commit on my pci/vpd branch, and this patch applies there. Duh. Anyway, I squashed this into that fix to avoid a bisection hole and updated pci/vpd and my "next" branch > > --- > > drivers/pci/vpd.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > > index 5108bbd20..a4fc4d069 100644 > > --- a/drivers/pci/vpd.c > > +++ b/drivers/pci/vpd.c > > @@ -96,14 +96,14 @@ static size_t pci_vpd_size(struct pci_dev *dev) > > return off ?: PCI_VPD_SZ_INVALID; > > } > > > > -static bool pci_vpd_available(struct pci_dev *dev) > > +static bool pci_vpd_available(struct pci_dev *dev, bool check_size) > > { > > struct pci_vpd *vpd = &dev->vpd; > > > > if (!vpd->cap) > > return false; > > > > - if (vpd->len == 0) { > > + if (vpd->len == 0 && check_size) { > > vpd->len = pci_vpd_size(dev); > > if (vpd->len == PCI_VPD_SZ_INVALID) { > > vpd->cap = 0; > > @@ -156,17 +156,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > > void *arg, bool check_size) > > { > > struct pci_vpd *vpd = &dev->vpd; > > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > > + unsigned int max_len; > > int ret = 0; > > loff_t end = pos + count; > > u8 *buf = arg; > > > > - if (!pci_vpd_available(dev)) > > + if (!pci_vpd_available(dev, check_size)) > > return -ENODEV; > > > > if (pos < 0) > > return -EINVAL; > > > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > > + > > if (pos >= max_len) > > return 0; > > > > @@ -218,17 +220,19 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > > const void *arg, bool check_size) > > { > > struct pci_vpd *vpd = &dev->vpd; > > - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > > + unsigned int max_len; > > const u8 *buf = arg; > > loff_t end = pos + count; > > int ret = 0; > > > > - if (!pci_vpd_available(dev)) > > + if (!pci_vpd_available(dev, check_size)) > > return -ENODEV; > > > > if (pos < 0 || (pos & 3) || (count & 3)) > > return -EINVAL; > > > > + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; > > + > > if (end > max_len) > > return -EINVAL; > > > > @@ -312,7 +316,7 @@ void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size) > > void *buf; > > int cnt; > > > > - if (!pci_vpd_available(dev)) > > + if (!pci_vpd_available(dev, true)) > > return ERR_PTR(-ENODEV); > > > > len = dev->vpd.len; > > -- > > 2.33.0 > >
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index 5108bbd20..a4fc4d069 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -96,14 +96,14 @@ static size_t pci_vpd_size(struct pci_dev *dev) return off ?: PCI_VPD_SZ_INVALID; } -static bool pci_vpd_available(struct pci_dev *dev) +static bool pci_vpd_available(struct pci_dev *dev, bool check_size) { struct pci_vpd *vpd = &dev->vpd; if (!vpd->cap) return false; - if (vpd->len == 0) { + if (vpd->len == 0 && check_size) { vpd->len = pci_vpd_size(dev); if (vpd->len == PCI_VPD_SZ_INVALID) { vpd->cap = 0; @@ -156,17 +156,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, void *arg, bool check_size) { struct pci_vpd *vpd = &dev->vpd; - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; + unsigned int max_len; int ret = 0; loff_t end = pos + count; u8 *buf = arg; - if (!pci_vpd_available(dev)) + if (!pci_vpd_available(dev, check_size)) return -ENODEV; if (pos < 0) return -EINVAL; + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; + if (pos >= max_len) return 0; @@ -218,17 +220,19 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, const void *arg, bool check_size) { struct pci_vpd *vpd = &dev->vpd; - unsigned int max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; + unsigned int max_len; const u8 *buf = arg; loff_t end = pos + count; int ret = 0; - if (!pci_vpd_available(dev)) + if (!pci_vpd_available(dev, check_size)) return -ENODEV; if (pos < 0 || (pos & 3) || (count & 3)) return -EINVAL; + max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE; + if (end > max_len) return -EINVAL; @@ -312,7 +316,7 @@ void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size) void *buf; int cnt; - if (!pci_vpd_available(dev)) + if (!pci_vpd_available(dev, true)) return ERR_PTR(-ENODEV); len = dev->vpd.len;
Recent bug fix 00e1a5d21b4f ("PCI/VPD: Defer VPD sizing until first access") interferes with the original change, resulting in a stack overflow. The following fix has been successfully tested by Qian and myself. Fixes: 80484b7f8db1 ("PCI/VPD: Use pci_read_vpd_any() in pci_vpd_size()") Reported-by: Qian Cai <quic_qiancai@quicinc.com> Tested-by: Qian Cai <quic_qiancai@quicinc.com> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/vpd.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)