| Message ID | 20161125153032.14617-5-oliver@schinagl.nl |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > In the current net stack, we have a few functions to get and set > the "ethaddr" and "ethNaddr" environment variables, which use magic > values to get and set these environment variables. Remove the magicness > of the buffer by defining it proper and also check the input for its > length. > > Additionally use the define in fdt parser where the ethaddr variables > are also used. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > common/fdt_support.c | 2 +- > include/net.h | 1 + > net/eth_common.c | 12 ++++++++---- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index b082662..89e6e47 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt) > { > int i, j, prop; > char *tmp, *end; > - char mac[16]; > + char mac[ETH_ENETADDR_LEN]; > const char *path; > unsigned char mac_addr[ARP_HLEN]; > int offset; > diff --git a/include/net.h b/include/net.h > index 9cd7870..2534913 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -243,6 +243,7 @@ void eth_set_current(void); /* set nterface to ethcur var */ > > int eth_get_dev_index(void); /* get the device index */ > void eth_parse_enetaddr(const char *addr, uchar *enetaddr); > +#define ETH_ENETADDR_LEN 32 I'd prefer a clearer name here. Maybe ETH_ENETADDR_ENV_NAME_LEN? > int eth_getenv_enetaddr(const char *name, uchar *enetaddr); > int eth_setenv_enetaddr(const char *name, const uchar *enetaddr); > > diff --git a/net/eth_common.c b/net/eth_common.c > index e9d3c66..079be89 100644 > --- a/net/eth_common.c > +++ b/net/eth_common.c > @@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr) > int eth_getenv_enetaddr_by_index(const char *base_name, int index, > uchar *enetaddr) > { > - char enetvar[32]; > - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); > + char enetvar[ETH_ENETADDR_LEN]; > + > + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", > + base_name, index); > return eth_getenv_enetaddr(enetvar, enetaddr); > } > > int eth_setenv_enetaddr_by_index(const char *base_name, int index, > uchar *enetaddr) > { > - char enetvar[32]; > - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); > + char enetvar[ETH_ENETADDR_LEN]; > + > + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", > + base_name, index); > return eth_setenv_enetaddr(enetvar, enetaddr); > } > > -- > 2.10.2 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hey Joe, On 30-11-16 20:20, Joe Hershberger wrote: > On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: >> In the current net stack, we have a few functions to get and set >> the "ethaddr" and "ethNaddr" environment variables, which use magic >> values to get and set these environment variables. Remove the magicness >> of the buffer by defining it proper and also check the input for its >> length. >> >> Additionally use the define in fdt parser where the ethaddr variables >> are also used. >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> --- >> common/fdt_support.c | 2 +- >> include/net.h | 1 + >> net/eth_common.c | 12 ++++++++---- >> 3 files changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/common/fdt_support.c b/common/fdt_support.c >> index b082662..89e6e47 100644 >> --- a/common/fdt_support.c >> +++ b/common/fdt_support.c >> @@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt) >> { >> int i, j, prop; >> char *tmp, *end; >> - char mac[16]; >> + char mac[ETH_ENETADDR_LEN]; >> const char *path; >> unsigned char mac_addr[ARP_HLEN]; >> int offset; >> diff --git a/include/net.h b/include/net.h >> index 9cd7870..2534913 100644 >> --- a/include/net.h >> +++ b/include/net.h >> @@ -243,6 +243,7 @@ void eth_set_current(void); /* set nterface to ethcur var */ >> >> int eth_get_dev_index(void); /* get the device index */ >> void eth_parse_enetaddr(const char *addr, uchar *enetaddr); >> +#define ETH_ENETADDR_LEN 32 > I'd prefer a clearer name here. I'd agree, but > > Maybe ETH_ENETADDR_ENV_NAME_LEN? I do find it to be quite a long name. Which makes the whole 80 charcters per line a thing. But done :) > >> int eth_getenv_enetaddr(const char *name, uchar *enetaddr); >> int eth_setenv_enetaddr(const char *name, const uchar *enetaddr); >> >> diff --git a/net/eth_common.c b/net/eth_common.c >> index e9d3c66..079be89 100644 >> --- a/net/eth_common.c >> +++ b/net/eth_common.c >> @@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr) >> int eth_getenv_enetaddr_by_index(const char *base_name, int index, >> uchar *enetaddr) >> { >> - char enetvar[32]; >> - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); >> + char enetvar[ETH_ENETADDR_LEN]; >> + >> + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", >> + base_name, index); >> return eth_getenv_enetaddr(enetvar, enetaddr); >> } >> >> int eth_setenv_enetaddr_by_index(const char *base_name, int index, >> uchar *enetaddr) >> { >> - char enetvar[32]; >> - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); >> + char enetvar[ETH_ENETADDR_LEN]; >> + >> + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", >> + base_name, index); >> return eth_setenv_enetaddr(enetvar, enetaddr); >> } >> >> -- >> 2.10.2 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot
diff --git a/common/fdt_support.c b/common/fdt_support.c index b082662..89e6e47 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt) { int i, j, prop; char *tmp, *end; - char mac[16]; + char mac[ETH_ENETADDR_LEN]; const char *path; unsigned char mac_addr[ARP_HLEN]; int offset; diff --git a/include/net.h b/include/net.h index 9cd7870..2534913 100644 --- a/include/net.h +++ b/include/net.h @@ -243,6 +243,7 @@ void eth_set_current(void); /* set nterface to ethcur var */ int eth_get_dev_index(void); /* get the device index */ void eth_parse_enetaddr(const char *addr, uchar *enetaddr); +#define ETH_ENETADDR_LEN 32 int eth_getenv_enetaddr(const char *name, uchar *enetaddr); int eth_setenv_enetaddr(const char *name, const uchar *enetaddr); diff --git a/net/eth_common.c b/net/eth_common.c index e9d3c66..079be89 100644 --- a/net/eth_common.c +++ b/net/eth_common.c @@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr) int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr) { - char enetvar[32]; - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + char enetvar[ETH_ENETADDR_LEN]; + + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", + base_name, index); return eth_getenv_enetaddr(enetvar, enetaddr); } int eth_setenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr) { - char enetvar[32]; - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + char enetvar[ETH_ENETADDR_LEN]; + + snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr", + base_name, index); return eth_setenv_enetaddr(enetvar, enetaddr); }
In the current net stack, we have a few functions to get and set the "ethaddr" and "ethNaddr" environment variables, which use magic values to get and set these environment variables. Remove the magicness of the buffer by defining it proper and also check the input for its length. Additionally use the define in fdt parser where the ethaddr variables are also used. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> --- common/fdt_support.c | 2 +- include/net.h | 1 + net/eth_common.c | 12 ++++++++---- 3 files changed, 10 insertions(+), 5 deletions(-)