| Message ID | 20240409131655.59157-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] api: add missing cookie checks for network access | expand |
On Tue, Apr 09, 2024 at 03:16:55PM +0200, Heinrich Schuchardt wrote: > dev_write_net() and dev_read_net() should validate the provided cookie. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > api/api_net.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/api/api_net.c b/api/api_net.c > index 7515c26e8b4..0b931a80787 100644 > --- a/api/api_net.c > +++ b/api/api_net.c > @@ -72,14 +72,16 @@ int dev_enum_net(struct device_info *di) > > int dev_write_net(void *cookie, void *buf, int len) > { > - /* XXX verify that cookie points to a valid net device??? */ > + if (!dev_valid_net(cookie)) > + return API_ENODEV; > > return eth_send(buf, len); > } > > int dev_read_net(void *cookie, void *buf, int len) > { > - /* XXX verify that cookie points to a valid net device??? */ > + if (!dev_valid_net(cookie)) > + return API_ENODEV; > > return eth_receive(buf, len); > } Is this right? Probably. But what triggered looking in to this to start with? I don't think anything is enabling the API support these days even.
On 4/10/24 02:43, Tom Rini wrote: > On Tue, Apr 09, 2024 at 03:16:55PM +0200, Heinrich Schuchardt wrote: > >> dev_write_net() and dev_read_net() should validate the provided cookie. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> api/api_net.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/api/api_net.c b/api/api_net.c >> index 7515c26e8b4..0b931a80787 100644 >> --- a/api/api_net.c >> +++ b/api/api_net.c >> @@ -72,14 +72,16 @@ int dev_enum_net(struct device_info *di) >> >> int dev_write_net(void *cookie, void *buf, int len) >> { >> - /* XXX verify that cookie points to a valid net device??? */ >> + if (!dev_valid_net(cookie)) >> + return API_ENODEV; >> >> return eth_send(buf, len); >> } >> >> int dev_read_net(void *cookie, void *buf, int len) >> { >> - /* XXX verify that cookie points to a valid net device??? */ >> + if (!dev_valid_net(cookie)) >> + return API_ENODEV; >> >> return eth_receive(buf, len); >> } > > Is this right? Probably. But what triggered looking in to this to start > with? I don't think anything is enabling the API support these days > even. > We should either properly test the API in our CI or or remove it. What once was done via the API could be done via an EFI payload in a more portable way today. Best regards Heinrich
On Wed, Apr 10, 2024 at 07:13:21AM +0200, Heinrich Schuchardt wrote: > On 4/10/24 02:43, Tom Rini wrote: > > On Tue, Apr 09, 2024 at 03:16:55PM +0200, Heinrich Schuchardt wrote: > > > > > dev_write_net() and dev_read_net() should validate the provided cookie. > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > --- > > > api/api_net.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/api/api_net.c b/api/api_net.c > > > index 7515c26e8b4..0b931a80787 100644 > > > --- a/api/api_net.c > > > +++ b/api/api_net.c > > > @@ -72,14 +72,16 @@ int dev_enum_net(struct device_info *di) > > > int dev_write_net(void *cookie, void *buf, int len) > > > { > > > - /* XXX verify that cookie points to a valid net device??? */ > > > + if (!dev_valid_net(cookie)) > > > + return API_ENODEV; > > > return eth_send(buf, len); > > > } > > > int dev_read_net(void *cookie, void *buf, int len) > > > { > > > - /* XXX verify that cookie points to a valid net device??? */ > > > + if (!dev_valid_net(cookie)) > > > + return API_ENODEV; > > > return eth_receive(buf, len); > > > } > > > > Is this right? Probably. But what triggered looking in to this to start > > with? I don't think anything is enabling the API support these days > > even. > > > > We should either properly test the API in our CI or or remove it. > > What once was done via the API could be done via an EFI payload in a more > portable way today. Yes, we should indeed likely remove it.
diff --git a/api/api_net.c b/api/api_net.c index 7515c26e8b4..0b931a80787 100644 --- a/api/api_net.c +++ b/api/api_net.c @@ -72,14 +72,16 @@ int dev_enum_net(struct device_info *di) int dev_write_net(void *cookie, void *buf, int len) { - /* XXX verify that cookie points to a valid net device??? */ + if (!dev_valid_net(cookie)) + return API_ENODEV; return eth_send(buf, len); } int dev_read_net(void *cookie, void *buf, int len) { - /* XXX verify that cookie points to a valid net device??? */ + if (!dev_valid_net(cookie)) + return API_ENODEV; return eth_receive(buf, len); }
dev_write_net() and dev_read_net() should validate the provided cookie. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- api/api_net.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)