| Message ID | 20181126200435.23408-16-minyard@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix/add vmstate handling in some I2C code | expand |
On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote: > > From: Philippe Mathieu-Daudé <philmd@redhat.com> > > Calling smbus_eeprom_init() with more than 8 EEPROMs would lead to a > heap overflow. > Replace the '8' magic number by a definition, and check no more than > this number are created. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > hw/i2c/smbus_eeprom.c | 13 +++++++++++-- > include/hw/i2c/smbus_eeprom.h | 4 +++- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index 942057dc10..a0dcadbd60 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -23,6 +23,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > #include "hw/hw.h" > #include "hw/boards.h" > #include "hw/i2c/i2c.h" > @@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > qdev_init_nofail(dev); > } > > -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > +void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom, > const uint8_t *eeprom_spd, int eeprom_spd_size) > { > int i; > + uint8_t *eeprom_buf; > + > + if (nb_eeprom > SMBUS_EEPROM_MAX) { > + error_report("At most %u EEPROM are supported on a SMBus.", > + SMBUS_EEPROM_MAX); > + exit(1); You could also just assert(), since this would be a QEMU bug (all callers use fixed values for this argument). > + } > + > /* XXX: make this persistent */ > - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); > + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); So if we allocate N buffers as the caller requests, what is the thing that means that more than 8 won't work ? We've now changed from allocating always 8 lots of the EEPROM size to possibly allocating fewer than that. How does the code in the device know what the size of the buffer we're passing as the "data" property is now? We don't pass it the number of EEPROMs as a property. > if (eeprom_spd_size > 0) { > memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size); > } > diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h > index 46fb1a37d6..8436200599 100644 > --- a/include/hw/i2c/smbus_eeprom.h > +++ b/include/hw/i2c/smbus_eeprom.h > @@ -25,8 +25,10 @@ > > #include "hw/i2c/i2c.h" > > +#define SMBUS_EEPROM_MAX 8 > + > void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf); > -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom, > +void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom, > const uint8_t *eeprom_spd, int size); thanks -- PMM
On 11/30/18 11:39 AM, Peter Maydell wrote: > On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote: >> From: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> Calling smbus_eeprom_init() with more than 8 EEPROMs would lead to a >> heap overflow. >> Replace the '8' magic number by a definition, and check no more than >> this number are created. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> --- >> hw/i2c/smbus_eeprom.c | 13 +++++++++++-- >> include/hw/i2c/smbus_eeprom.h | 4 +++- >> 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >> index 942057dc10..a0dcadbd60 100644 >> --- a/hw/i2c/smbus_eeprom.c >> +++ b/hw/i2c/smbus_eeprom.c >> @@ -23,6 +23,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> #include "hw/hw.h" >> #include "hw/boards.h" >> #include "hw/i2c/i2c.h" >> @@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) >> qdev_init_nofail(dev); >> } >> >> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >> +void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom, >> const uint8_t *eeprom_spd, int eeprom_spd_size) >> { >> int i; >> + uint8_t *eeprom_buf; >> + >> + if (nb_eeprom > SMBUS_EEPROM_MAX) { >> + error_report("At most %u EEPROM are supported on a SMBus.", >> + SMBUS_EEPROM_MAX); >> + exit(1); > You could also just assert(), since this would be a > QEMU bug (all callers use fixed values for this argument). I'm fine either way. Philippe? >> + } >> + >> /* XXX: make this persistent */ >> - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); >> + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); > So if we allocate N buffers as the caller requests, what > is the thing that means that more than 8 won't work ? > > We've now changed from allocating always 8 lots of > the EEPROM size to possibly allocating fewer than that. > How does the code in the device know what the size > of the buffer we're passing as the "data" property is > now? We don't pass it the number of EEPROMs as a property. It doesn't have to. Each EEPROM is 256 bytes and that's all the data it has. But this whole thing is confusing, I agree. The more I look at this particular file, the less I like it. But the caller is basically saying: "I need N EEPROMs, here's the initialization data". That's not really very flexible, IMHO the EEPROM devices should be created individually using standard qemu methods. I'm tempted to rewrite this whole thing to make it cleaner. -corey >> if (eeprom_spd_size > 0) { >> memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size); >> } >> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h >> index 46fb1a37d6..8436200599 100644 >> --- a/include/hw/i2c/smbus_eeprom.h >> +++ b/include/hw/i2c/smbus_eeprom.h >> @@ -25,8 +25,10 @@ >> >> #include "hw/i2c/i2c.h" >> >> +#define SMBUS_EEPROM_MAX 8 >> + >> void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf); >> -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom, >> +void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom, >> const uint8_t *eeprom_spd, int size); > thanks > -- PMM
On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote: > > On 11/30/18 11:39 AM, Peter Maydell wrote: > > On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote: > >> From: Philippe Mathieu-Daudé <philmd@redhat.com> > >> /* XXX: make this persistent */ > >> - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); > >> + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); > > So if we allocate N buffers as the caller requests, what > > is the thing that means that more than 8 won't work ? > > > > We've now changed from allocating always 8 lots of > > the EEPROM size to possibly allocating fewer than that. > > How does the code in the device know what the size > > of the buffer we're passing as the "data" property is > > now? We don't pass it the number of EEPROMs as a property. > > It doesn't have to. Each EEPROM is 256 bytes and that's all the data > it has. > > But this whole thing is confusing, I agree. The more I look at this > particular file, the less I like it. But the caller is basically saying: > "I need N EEPROMs, here's the initialization data". That's not > really very flexible, IMHO the EEPROM devices should be created > individually using standard qemu methods. Oh, yes, I see now. We pass in a block of N * 256 bytes, and the function then hands a pointer to offsets 0, 256, ... to each device it creates. I definitely don't see why we need to say "only a maximum of 8" now, though -- where does that limit come from? If we get passed in an arbitrary number of EEPROMs X, and we allocate 256 * X bytes, and create X devices which each get one slice of the buffer, what goes wrong when X > 8 ? > I'm tempted to rewrite this whole thing to make it cleaner. It's certainly pretty awkward code. I think personally given this patchset is already pretty big I'd go for getting it into master first and then doing a followup with further cleanup. thanks -- PMM
On 1/12/18 12:57, Peter Maydell wrote: > On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote: >> >> On 11/30/18 11:39 AM, Peter Maydell wrote: >>> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote: >>>> From: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> /* XXX: make this persistent */ >>>> - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); >>>> + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); >>> So if we allocate N buffers as the caller requests, what >>> is the thing that means that more than 8 won't work ? >>> >>> We've now changed from allocating always 8 lots of >>> the EEPROM size to possibly allocating fewer than that. >>> How does the code in the device know what the size >>> of the buffer we're passing as the "data" property is >>> now? We don't pass it the number of EEPROMs as a property. >> >> It doesn't have to. Each EEPROM is 256 bytes and that's all the data >> it has. >> >> But this whole thing is confusing, I agree. The more I look at this >> particular file, the less I like it. But the caller is basically saying: >> "I need N EEPROMs, here's the initialization data". That's not >> really very flexible, IMHO the EEPROM devices should be created >> individually using standard qemu methods. > > Oh, yes, I see now. We pass in a block of N * 256 bytes, and > the function then hands a pointer to offsets 0, 256, ... > to each device it creates. > > I definitely don't see why we need to say "only a maximum of > 8" now, though -- where does that limit come from? If we > get passed in an arbitrary number of EEPROMs X, and we allocate > 256 * X bytes, and create X devices which each get one slice of > the buffer, what goes wrong when X > 8 ? > >> I'm tempted to rewrite this whole thing to make it cleaner. > > It's certainly pretty awkward code. I think personally given > this patchset is already pretty big I'd go for getting it into > master first and then doing a followup with further cleanup. As suggested Peter, I'd drop this patch (in favor of a later clean rewrite) and simply add an assert(). Regards, Phil.
On 12/1/18 11:43 AM, Philippe Mathieu-Daudé wrote: > On 1/12/18 12:57, Peter Maydell wrote: >> On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote: >>> On 11/30/18 11:39 AM, Peter Maydell wrote: >>>> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote: >>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> /* XXX: make this persistent */ >>>>> - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); >>>>> + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); >>>> So if we allocate N buffers as the caller requests, what >>>> is the thing that means that more than 8 won't work ? >>>> >>>> We've now changed from allocating always 8 lots of >>>> the EEPROM size to possibly allocating fewer than that. >>>> How does the code in the device know what the size >>>> of the buffer we're passing as the "data" property is >>>> now? We don't pass it the number of EEPROMs as a property. >>> It doesn't have to. Each EEPROM is 256 bytes and that's all the data >>> it has. >>> >>> But this whole thing is confusing, I agree. The more I look at this >>> particular file, the less I like it. But the caller is basically saying: >>> "I need N EEPROMs, here's the initialization data". That's not >>> really very flexible, IMHO the EEPROM devices should be created >>> individually using standard qemu methods. >> Oh, yes, I see now. We pass in a block of N * 256 bytes, and >> the function then hands a pointer to offsets 0, 256, ... >> to each device it creates. >> >> I definitely don't see why we need to say "only a maximum of >> 8" now, though -- where does that limit come from? I missed this question. The limit comes from the chip design. It has 3 pins that set the bottom 3 bit of the I2C address, the address can range from 0x50 to 0x57. So it's not arbitrary, I guess, but I don't see a strong reason to put the limitation there. -corey >> If we >> get passed in an arbitrary number of EEPROMs X, and we allocate >> 256 * X bytes, and create X devices which each get one slice of >> the buffer, what goes wrong when X > 8 ? >> >>> I'm tempted to rewrite this whole thing to make it cleaner. >> It's certainly pretty awkward code. I think personally given >> this patchset is already pretty big I'd go for getting it into >> master first and then doing a followup with further cleanup. > As suggested Peter, I'd drop this patch (in favor of a later clean > rewrite) and simply add an assert(). > > Regards, > > Phil.
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 942057dc10..a0dcadbd60 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -23,6 +23,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "hw/hw.h" #include "hw/boards.h" #include "hw/i2c/i2c.h" @@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) qdev_init_nofail(dev); } -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, +void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom, const uint8_t *eeprom_spd, int eeprom_spd_size) { int i; + uint8_t *eeprom_buf; + + if (nb_eeprom > SMBUS_EEPROM_MAX) { + error_report("At most %u EEPROM are supported on a SMBus.", + SMBUS_EEPROM_MAX); + exit(1); + } + /* XXX: make this persistent */ - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); if (eeprom_spd_size > 0) { memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size); } diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h index 46fb1a37d6..8436200599 100644 --- a/include/hw/i2c/smbus_eeprom.h +++ b/include/hw/i2c/smbus_eeprom.h @@ -25,8 +25,10 @@ #include "hw/i2c/i2c.h" +#define SMBUS_EEPROM_MAX 8 + void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf); -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom, +void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom, const uint8_t *eeprom_spd, int size); #endif