| Message ID | 20201209160956.32456-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
On 09.12.2020 17:09, Juergen Gross wrote: > +static const struct hypfs_entry *hypfs_dyndir_enter( > + const struct hypfs_entry *entry) > +{ > + const struct hypfs_dyndir_id *data; > + > + data = hypfs_get_dyndata(); > + > + /* Use template with original enter function. */ > + return data->template->e.funcs->enter(&data->template->e); > +} At the example of this (applies to other uses as well): I realize hypfs_get_dyndata() asserts that the pointer is non-NULL, but according to the bottom of ./CODING_STYLE this may not be enough when considering the implications of a NULL deref in the context of a PV guest. Even this living behind a sysctl doesn't really help, both because via XSM not fully privileged domains can be granted access, and because speculation may still occur all the way into here. (I'll send a patch to address the latter aspect in a few minutes.) While likely we have numerous existing examples with similar problems, I guess in new code we'd better be as defensive as possible. > +/* > + * Fill dyndata with a dynamically generated directory based on a template > + * and a numerical id. > + * Needs to be kept in sync with hypfs_read_dyndir_id_entry() regarding the > + * name generated. > + */ > +struct hypfs_entry *hypfs_gen_dyndir_id_entry( > + const struct hypfs_entry_dir *template, unsigned int id, void *data) > +{ s/directory/entry/ in the comment (and as a I realize only now then also for hypfs_read_dyndir_id_entry())? Jan
On 17.12.20 12:28, Jan Beulich wrote: > On 09.12.2020 17:09, Juergen Gross wrote: >> +static const struct hypfs_entry *hypfs_dyndir_enter( >> + const struct hypfs_entry *entry) >> +{ >> + const struct hypfs_dyndir_id *data; >> + >> + data = hypfs_get_dyndata(); >> + >> + /* Use template with original enter function. */ >> + return data->template->e.funcs->enter(&data->template->e); >> +} > > At the example of this (applies to other uses as well): I realize > hypfs_get_dyndata() asserts that the pointer is non-NULL, but > according to the bottom of ./CODING_STYLE this may not be enough > when considering the implications of a NULL deref in the context > of a PV guest. Even this living behind a sysctl doesn't really > help, both because via XSM not fully privileged domains can be > granted access, and because speculation may still occur all the > way into here. (I'll send a patch to address the latter aspect in > a few minutes.) While likely we have numerous existing examples > with similar problems, I guess in new code we'd better be as > defensive as possible. What do you suggest? BUG_ON()? You are aware that this is nothing a user can influence, so it would be a clear coding error in the hypervisor? > >> +/* >> + * Fill dyndata with a dynamically generated directory based on a template >> + * and a numerical id. >> + * Needs to be kept in sync with hypfs_read_dyndir_id_entry() regarding the >> + * name generated. >> + */ >> +struct hypfs_entry *hypfs_gen_dyndir_id_entry( >> + const struct hypfs_entry_dir *template, unsigned int id, void *data) >> +{ > > s/directory/entry/ in the comment (and as a I realize only now > then also for hypfs_read_dyndir_id_entry())? Oh, indeed. Juergen
On 17.12.2020 12:32, Jürgen Groß wrote: > On 17.12.20 12:28, Jan Beulich wrote: >> On 09.12.2020 17:09, Juergen Gross wrote: >>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>> + const struct hypfs_entry *entry) >>> +{ >>> + const struct hypfs_dyndir_id *data; >>> + >>> + data = hypfs_get_dyndata(); >>> + >>> + /* Use template with original enter function. */ >>> + return data->template->e.funcs->enter(&data->template->e); >>> +} >> >> At the example of this (applies to other uses as well): I realize >> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >> according to the bottom of ./CODING_STYLE this may not be enough >> when considering the implications of a NULL deref in the context >> of a PV guest. Even this living behind a sysctl doesn't really >> help, both because via XSM not fully privileged domains can be >> granted access, and because speculation may still occur all the >> way into here. (I'll send a patch to address the latter aspect in >> a few minutes.) While likely we have numerous existing examples >> with similar problems, I guess in new code we'd better be as >> defensive as possible. > > What do you suggest? BUG_ON()? Well, BUG_ON() would be a step in the right direction, converting privilege escalation to DoS. The question is if we can't do better here, gracefully failing in such a case (the usual pair of ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit here, at least not directly). > You are aware that this is nothing a user can influence, so it would > be a clear coding error in the hypervisor? A user (or guest) can't arrange for there to be a NULL pointer, but if there is one that can be run into here, this would still require an XSA afaict. Jan
On 17.12.20 13:14, Jan Beulich wrote: > On 17.12.2020 12:32, Jürgen Groß wrote: >> On 17.12.20 12:28, Jan Beulich wrote: >>> On 09.12.2020 17:09, Juergen Gross wrote: >>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>> + const struct hypfs_entry *entry) >>>> +{ >>>> + const struct hypfs_dyndir_id *data; >>>> + >>>> + data = hypfs_get_dyndata(); >>>> + >>>> + /* Use template with original enter function. */ >>>> + return data->template->e.funcs->enter(&data->template->e); >>>> +} >>> >>> At the example of this (applies to other uses as well): I realize >>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>> according to the bottom of ./CODING_STYLE this may not be enough >>> when considering the implications of a NULL deref in the context >>> of a PV guest. Even this living behind a sysctl doesn't really >>> help, both because via XSM not fully privileged domains can be >>> granted access, and because speculation may still occur all the >>> way into here. (I'll send a patch to address the latter aspect in >>> a few minutes.) While likely we have numerous existing examples >>> with similar problems, I guess in new code we'd better be as >>> defensive as possible. >> >> What do you suggest? BUG_ON()? > > Well, BUG_ON() would be a step in the right direction, converting > privilege escalation to DoS. The question is if we can't do better > here, gracefully failing in such a case (the usual pair of > ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit > here, at least not directly). > >> You are aware that this is nothing a user can influence, so it would >> be a clear coding error in the hypervisor? > > A user (or guest) can't arrange for there to be a NULL pointer, > but if there is one that can be run into here, this would still > require an XSA afaict. I still don't see how this could happen without a major coding bug, which IMO wouldn't go unnoticed during a really brief test (this is the reason for ASSERT() in hypfs_get_dyndata() after all). Its not as if the control flow would allow many different ways to reach any of the hypfs_get_dyndata() calls. I can add security checks at the appropriate places, but I think this would be just dead code. OTOH if you are feeling strong here lets go with it. Juergen
On 18.12.2020 09:57, Jürgen Groß wrote: > On 17.12.20 13:14, Jan Beulich wrote: >> On 17.12.2020 12:32, Jürgen Groß wrote: >>> On 17.12.20 12:28, Jan Beulich wrote: >>>> On 09.12.2020 17:09, Juergen Gross wrote: >>>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>>> + const struct hypfs_entry *entry) >>>>> +{ >>>>> + const struct hypfs_dyndir_id *data; >>>>> + >>>>> + data = hypfs_get_dyndata(); >>>>> + >>>>> + /* Use template with original enter function. */ >>>>> + return data->template->e.funcs->enter(&data->template->e); >>>>> +} >>>> >>>> At the example of this (applies to other uses as well): I realize >>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>>> according to the bottom of ./CODING_STYLE this may not be enough >>>> when considering the implications of a NULL deref in the context >>>> of a PV guest. Even this living behind a sysctl doesn't really >>>> help, both because via XSM not fully privileged domains can be >>>> granted access, and because speculation may still occur all the >>>> way into here. (I'll send a patch to address the latter aspect in >>>> a few minutes.) While likely we have numerous existing examples >>>> with similar problems, I guess in new code we'd better be as >>>> defensive as possible. >>> >>> What do you suggest? BUG_ON()? >> >> Well, BUG_ON() would be a step in the right direction, converting >> privilege escalation to DoS. The question is if we can't do better >> here, gracefully failing in such a case (the usual pair of >> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit >> here, at least not directly). >> >>> You are aware that this is nothing a user can influence, so it would >>> be a clear coding error in the hypervisor? >> >> A user (or guest) can't arrange for there to be a NULL pointer, >> but if there is one that can be run into here, this would still >> require an XSA afaict. > > I still don't see how this could happen without a major coding bug, > which IMO wouldn't go unnoticed during a really brief test (this is > the reason for ASSERT() in hypfs_get_dyndata() after all). True. Yet the NULL derefs wouldn't go unnoticed either. > Its not as if the control flow would allow many different ways to reach > any of the hypfs_get_dyndata() calls. I'm not convinced of this - this is a non-static function, and the call patch 8 adds (just to take an example) is not very obvious to have a guarantee that allocation did happen and was checked for success. Yes, in principle cpupool_gran_write() isn't supposed to be called in such a case, but it's the nature of bugs assumptions get broken. > I can add security checks at the appropriate places, but I think this > would be just dead code. OTOH if you are feeling strong here lets go > with it. Going with it isn't the only possible route. The other is to drop the ASSERT()s altogether. It simply seems to me that their addition is a half-hearted attempt when considering what was added to ./CODING_STYLE not all that long ago. Jan
On 18.12.20 10:09, Jan Beulich wrote: > On 18.12.2020 09:57, Jürgen Groß wrote: >> On 17.12.20 13:14, Jan Beulich wrote: >>> On 17.12.2020 12:32, Jürgen Groß wrote: >>>> On 17.12.20 12:28, Jan Beulich wrote: >>>>> On 09.12.2020 17:09, Juergen Gross wrote: >>>>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>>>> + const struct hypfs_entry *entry) >>>>>> +{ >>>>>> + const struct hypfs_dyndir_id *data; >>>>>> + >>>>>> + data = hypfs_get_dyndata(); >>>>>> + >>>>>> + /* Use template with original enter function. */ >>>>>> + return data->template->e.funcs->enter(&data->template->e); >>>>>> +} >>>>> >>>>> At the example of this (applies to other uses as well): I realize >>>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>>>> according to the bottom of ./CODING_STYLE this may not be enough >>>>> when considering the implications of a NULL deref in the context >>>>> of a PV guest. Even this living behind a sysctl doesn't really >>>>> help, both because via XSM not fully privileged domains can be >>>>> granted access, and because speculation may still occur all the >>>>> way into here. (I'll send a patch to address the latter aspect in >>>>> a few minutes.) While likely we have numerous existing examples >>>>> with similar problems, I guess in new code we'd better be as >>>>> defensive as possible. >>>> >>>> What do you suggest? BUG_ON()? >>> >>> Well, BUG_ON() would be a step in the right direction, converting >>> privilege escalation to DoS. The question is if we can't do better >>> here, gracefully failing in such a case (the usual pair of >>> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit >>> here, at least not directly). >>> >>>> You are aware that this is nothing a user can influence, so it would >>>> be a clear coding error in the hypervisor? >>> >>> A user (or guest) can't arrange for there to be a NULL pointer, >>> but if there is one that can be run into here, this would still >>> require an XSA afaict. >> >> I still don't see how this could happen without a major coding bug, >> which IMO wouldn't go unnoticed during a really brief test (this is >> the reason for ASSERT() in hypfs_get_dyndata() after all). > > True. Yet the NULL derefs wouldn't go unnoticed either. > >> Its not as if the control flow would allow many different ways to reach >> any of the hypfs_get_dyndata() calls. > > I'm not convinced of this - this is a non-static function, and the > call patch 8 adds (just to take an example) is not very obvious to > have a guarantee that allocation did happen and was checked for > success. Yes, in principle cpupool_gran_write() isn't supposed to > be called in such a case, but it's the nature of bugs assumptions > get broken. Yes, but we do have tons of assumptions like that. I don't think we should add tests for non-NULL pointers everywhere just because we happen to dereference something. Where do we stop? > >> I can add security checks at the appropriate places, but I think this >> would be just dead code. OTOH if you are feeling strong here lets go >> with it. > > Going with it isn't the only possible route. The other is to drop > the ASSERT()s altogether. It simply seems to me that their addition > is a half-hearted attempt when considering what was added to > ./CODING_STYLE not all that long ago. No. The ASSERT() is clearly an attempt to catch a programming error early. It is especially not trying to catch a situation which is thought to be possible. The situation should really never happen, and I'm not aware how it could happen without a weird code modification. Dropping the ASSERT() would really add risk to not notice a bug being introduced by a code modification. Juergen
On 18.12.2020 13:41, Jürgen Groß wrote: > On 18.12.20 10:09, Jan Beulich wrote: >> On 18.12.2020 09:57, Jürgen Groß wrote: >>> On 17.12.20 13:14, Jan Beulich wrote: >>>> On 17.12.2020 12:32, Jürgen Groß wrote: >>>>> On 17.12.20 12:28, Jan Beulich wrote: >>>>>> On 09.12.2020 17:09, Juergen Gross wrote: >>>>>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>>>>> + const struct hypfs_entry *entry) >>>>>>> +{ >>>>>>> + const struct hypfs_dyndir_id *data; >>>>>>> + >>>>>>> + data = hypfs_get_dyndata(); >>>>>>> + >>>>>>> + /* Use template with original enter function. */ >>>>>>> + return data->template->e.funcs->enter(&data->template->e); >>>>>>> +} >>>>>> >>>>>> At the example of this (applies to other uses as well): I realize >>>>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>>>>> according to the bottom of ./CODING_STYLE this may not be enough >>>>>> when considering the implications of a NULL deref in the context >>>>>> of a PV guest. Even this living behind a sysctl doesn't really >>>>>> help, both because via XSM not fully privileged domains can be >>>>>> granted access, and because speculation may still occur all the >>>>>> way into here. (I'll send a patch to address the latter aspect in >>>>>> a few minutes.) While likely we have numerous existing examples >>>>>> with similar problems, I guess in new code we'd better be as >>>>>> defensive as possible. >>>>> >>>>> What do you suggest? BUG_ON()? >>>> >>>> Well, BUG_ON() would be a step in the right direction, converting >>>> privilege escalation to DoS. The question is if we can't do better >>>> here, gracefully failing in such a case (the usual pair of >>>> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit >>>> here, at least not directly). >>>> >>>>> You are aware that this is nothing a user can influence, so it would >>>>> be a clear coding error in the hypervisor? >>>> >>>> A user (or guest) can't arrange for there to be a NULL pointer, >>>> but if there is one that can be run into here, this would still >>>> require an XSA afaict. >>> >>> I still don't see how this could happen without a major coding bug, >>> which IMO wouldn't go unnoticed during a really brief test (this is >>> the reason for ASSERT() in hypfs_get_dyndata() after all). >> >> True. Yet the NULL derefs wouldn't go unnoticed either. >> >>> Its not as if the control flow would allow many different ways to reach >>> any of the hypfs_get_dyndata() calls. >> >> I'm not convinced of this - this is a non-static function, and the >> call patch 8 adds (just to take an example) is not very obvious to >> have a guarantee that allocation did happen and was checked for >> success. Yes, in principle cpupool_gran_write() isn't supposed to >> be called in such a case, but it's the nature of bugs assumptions >> get broken. > > Yes, but we do have tons of assumptions like that. I don't think we > should add tests for non-NULL pointers everywhere just because we > happen to dereference something. Where do we stop? > >> >>> I can add security checks at the appropriate places, but I think this >>> would be just dead code. OTOH if you are feeling strong here lets go >>> with it. >> >> Going with it isn't the only possible route. The other is to drop >> the ASSERT()s altogether. It simply seems to me that their addition >> is a half-hearted attempt when considering what was added to >> ./CODING_STYLE not all that long ago. > > No. The ASSERT() is clearly an attempt to catch a programming error > early. It is especially not trying to catch a situation which is thought > to be possible. The situation should really never happen, and I'm not > aware how it could happen without a weird code modification. > > Dropping the ASSERT() would really add risk to not notice a bug being > introduced by a code modification. Is this the case? Wouldn't the NULL be de-referenced almost immediately, and hence the bug be noticed right away anyway? I don't think it is typical for PV guests to have a valid mapping for address 0. Putting in place such a mapping could at least be a hint towards possible malice imo. Jan
On 18.12.20 10:09, Jan Beulich wrote: > On 18.12.2020 09:57, Jürgen Groß wrote: >> On 17.12.20 13:14, Jan Beulich wrote: >>> On 17.12.2020 12:32, Jürgen Groß wrote: >>>> On 17.12.20 12:28, Jan Beulich wrote: >>>>> On 09.12.2020 17:09, Juergen Gross wrote: >>>>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>>>> + const struct hypfs_entry *entry) >>>>>> +{ >>>>>> + const struct hypfs_dyndir_id *data; >>>>>> + >>>>>> + data = hypfs_get_dyndata(); >>>>>> + >>>>>> + /* Use template with original enter function. */ >>>>>> + return data->template->e.funcs->enter(&data->template->e); >>>>>> +} >>>>> >>>>> At the example of this (applies to other uses as well): I realize >>>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>>>> according to the bottom of ./CODING_STYLE this may not be enough >>>>> when considering the implications of a NULL deref in the context >>>>> of a PV guest. Even this living behind a sysctl doesn't really >>>>> help, both because via XSM not fully privileged domains can be >>>>> granted access, and because speculation may still occur all the >>>>> way into here. (I'll send a patch to address the latter aspect in >>>>> a few minutes.) While likely we have numerous existing examples >>>>> with similar problems, I guess in new code we'd better be as >>>>> defensive as possible. >>>> >>>> What do you suggest? BUG_ON()? >>> >>> Well, BUG_ON() would be a step in the right direction, converting >>> privilege escalation to DoS. The question is if we can't do better >>> here, gracefully failing in such a case (the usual pair of >>> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit >>> here, at least not directly). >>> >>>> You are aware that this is nothing a user can influence, so it would >>>> be a clear coding error in the hypervisor? >>> >>> A user (or guest) can't arrange for there to be a NULL pointer, >>> but if there is one that can be run into here, this would still >>> require an XSA afaict. >> >> I still don't see how this could happen without a major coding bug, >> which IMO wouldn't go unnoticed during a really brief test (this is >> the reason for ASSERT() in hypfs_get_dyndata() after all). > > True. Yet the NULL derefs wouldn't go unnoticed either. > >> Its not as if the control flow would allow many different ways to reach >> any of the hypfs_get_dyndata() calls. > > I'm not convinced of this - this is a non-static function, and the > call patch 8 adds (just to take an example) is not very obvious to > have a guarantee that allocation did happen and was checked for > success. Yes, in principle cpupool_gran_write() isn't supposed to > be called in such a case, but it's the nature of bugs assumptions > get broken. > >> I can add security checks at the appropriate places, but I think this >> would be just dead code. OTOH if you are feeling strong here lets go >> with it. > > Going with it isn't the only possible route. The other is to drop > the ASSERT()s altogether. It simply seems to me that their addition > is a half-hearted attempt when considering what was added to > ./CODING_STYLE not all that long ago. IMO The ASSERT()s are serving two purposes here: catching errors (which, as you stated already, might be catched later in any case), and documenting for the code reader that the condition they are testing should always be true and it a violation of it ought not to happen. I can drop the ASSERT() calls, but I think they should be kept due to their documentation aspect. Replacing them with setting a negative return value is possible and I will do it if you are insisting on it, but I still think this is not a good solution, as it is adding code for a situation which really should never happen (you could compare it to replacing ASSERT(spin_is_locked()) cases with returning an error). Juergen
On 18.01.2021 08:25, Jürgen Groß wrote: > On 18.12.20 10:09, Jan Beulich wrote: >> On 18.12.2020 09:57, Jürgen Groß wrote: >>> On 17.12.20 13:14, Jan Beulich wrote: >>>> On 17.12.2020 12:32, Jürgen Groß wrote: >>>>> On 17.12.20 12:28, Jan Beulich wrote: >>>>>> On 09.12.2020 17:09, Juergen Gross wrote: >>>>>>> +static const struct hypfs_entry *hypfs_dyndir_enter( >>>>>>> + const struct hypfs_entry *entry) >>>>>>> +{ >>>>>>> + const struct hypfs_dyndir_id *data; >>>>>>> + >>>>>>> + data = hypfs_get_dyndata(); >>>>>>> + >>>>>>> + /* Use template with original enter function. */ >>>>>>> + return data->template->e.funcs->enter(&data->template->e); >>>>>>> +} >>>>>> >>>>>> At the example of this (applies to other uses as well): I realize >>>>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but >>>>>> according to the bottom of ./CODING_STYLE this may not be enough >>>>>> when considering the implications of a NULL deref in the context >>>>>> of a PV guest. Even this living behind a sysctl doesn't really >>>>>> help, both because via XSM not fully privileged domains can be >>>>>> granted access, and because speculation may still occur all the >>>>>> way into here. (I'll send a patch to address the latter aspect in >>>>>> a few minutes.) While likely we have numerous existing examples >>>>>> with similar problems, I guess in new code we'd better be as >>>>>> defensive as possible. >>>>> >>>>> What do you suggest? BUG_ON()? >>>> >>>> Well, BUG_ON() would be a step in the right direction, converting >>>> privilege escalation to DoS. The question is if we can't do better >>>> here, gracefully failing in such a case (the usual pair of >>>> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit >>>> here, at least not directly). >>>> >>>>> You are aware that this is nothing a user can influence, so it would >>>>> be a clear coding error in the hypervisor? >>>> >>>> A user (or guest) can't arrange for there to be a NULL pointer, >>>> but if there is one that can be run into here, this would still >>>> require an XSA afaict. >>> >>> I still don't see how this could happen without a major coding bug, >>> which IMO wouldn't go unnoticed during a really brief test (this is >>> the reason for ASSERT() in hypfs_get_dyndata() after all). >> >> True. Yet the NULL derefs wouldn't go unnoticed either. >> >>> Its not as if the control flow would allow many different ways to reach >>> any of the hypfs_get_dyndata() calls. >> >> I'm not convinced of this - this is a non-static function, and the >> call patch 8 adds (just to take an example) is not very obvious to >> have a guarantee that allocation did happen and was checked for >> success. Yes, in principle cpupool_gran_write() isn't supposed to >> be called in such a case, but it's the nature of bugs assumptions >> get broken. >> >>> I can add security checks at the appropriate places, but I think this >>> would be just dead code. OTOH if you are feeling strong here lets go >>> with it. >> >> Going with it isn't the only possible route. The other is to drop >> the ASSERT()s altogether. It simply seems to me that their addition >> is a half-hearted attempt when considering what was added to >> ./CODING_STYLE not all that long ago. > > IMO The ASSERT()s are serving two purposes here: catching errors > (which, as you stated already, might be catched later in any case), > and documenting for the code reader that the condition they are > testing should always be true and it a violation of it ought not to > happen. > > I can drop the ASSERT() calls, but I think they should be kept due > to their documentation aspect. Well, okay, I can see your point. Keep them in then, despite us coming nowhere close to having similar NULL related asserts in all similar places, I believe. Jan
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 8faf65cea0..087c63b92f 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -356,6 +356,104 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) return entry->size; } +/* + * Fill the direntry for a dynamically generated directory. Especially the + * generated name needs to be kept in sync with hypfs_gen_dyndir_id_entry(). + */ +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, + unsigned int id, bool is_last, + XEN_GUEST_HANDLE_PARAM(void) *uaddr) +{ + struct xen_hypfs_dirlistentry direntry; + char name[HYPFS_DYNDIR_ID_NAMELEN]; + unsigned int e_namelen, e_len; + + e_namelen = snprintf(name, sizeof(name), template->e.name, id); + e_len = DIRENTRY_SIZE(e_namelen); + direntry.e.pad = 0; + direntry.e.type = template->e.type; + direntry.e.encoding = template->e.encoding; + direntry.e.content_len = template->e.funcs->getsize(&template->e); + direntry.e.max_write_len = template->e.max_size; + direntry.off_next = is_last ? 0 : e_len; + if ( copy_to_guest(*uaddr, &direntry, 1) ) + return -EFAULT; + if ( copy_to_guest_offset(*uaddr, DIRENTRY_NAME_OFF, name, + e_namelen + 1) ) + return -EFAULT; + + guest_handle_add_offset(*uaddr, e_len); + + return 0; +} + +static const struct hypfs_entry *hypfs_dyndir_enter( + const struct hypfs_entry *entry) +{ + const struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + /* Use template with original enter function. */ + return data->template->e.funcs->enter(&data->template->e); +} + +static struct hypfs_entry *hypfs_dyndir_findentry( + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) +{ + const struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + /* Use template with original findentry function. */ + return data->template->e.funcs->findentry(data->template, name, name_len); +} + +static int hypfs_read_dyndir(const struct hypfs_entry *entry, + XEN_GUEST_HANDLE_PARAM(void) uaddr) +{ + const struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + /* Use template with original read function. */ + return data->template->e.funcs->read(&data->template->e, uaddr); +} + +/* + * Fill dyndata with a dynamically generated directory based on a template + * and a numerical id. + * Needs to be kept in sync with hypfs_read_dyndir_id_entry() regarding the + * name generated. + */ +struct hypfs_entry *hypfs_gen_dyndir_id_entry( + const struct hypfs_entry_dir *template, unsigned int id, void *data) +{ + struct hypfs_dyndir_id *dyndata; + + dyndata = hypfs_get_dyndata(); + + dyndata->template = template; + dyndata->id = id; + dyndata->data = data; + snprintf(dyndata->name, sizeof(dyndata->name), template->e.name, id); + dyndata->dir = *template; + dyndata->dir.e.name = dyndata->name; + dyndata->dir.e.funcs = &dyndata->funcs; + dyndata->funcs = *template->e.funcs; + dyndata->funcs.enter = hypfs_dyndir_enter; + dyndata->funcs.findentry = hypfs_dyndir_findentry; + dyndata->funcs.read = hypfs_read_dyndir; + + return &dyndata->dir.e; +} + +unsigned int hypfs_dynid_entry_size(const struct hypfs_entry *template, + unsigned int id) +{ + return DIRENTRY_SIZE(snprintf(NULL, 0, template->name, id)); +} + int hypfs_read_dir(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr) { diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h index 4c469cbeb4..34073faff8 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -76,6 +76,17 @@ struct hypfs_entry_dir { struct list_head dirlist; }; +struct hypfs_dyndir_id { + struct hypfs_entry_dir dir; /* Modified copy of template. */ + struct hypfs_funcs funcs; /* Dynamic functions. */ + const struct hypfs_entry_dir *template; /* Template used. */ +#define HYPFS_DYNDIR_ID_NAMELEN 12 + char name[HYPFS_DYNDIR_ID_NAMELEN]; /* Name of hypfs entry. */ + + unsigned int id; /* Numerical id. */ + void *data; /* Data associated with id. */ +}; + #define HYPFS_DIR_INIT_FUNC(var, nam, fn) \ struct hypfs_entry_dir __read_mostly var = { \ .e.type = XEN_HYPFS_TYPE_DIR, \ @@ -186,6 +197,13 @@ void *hypfs_alloc_dyndata_size(unsigned long size); #define hypfs_alloc_dyndata(type) (type *)hypfs_alloc_dyndata_size(sizeof(type)) void *hypfs_get_dyndata(void); void hypfs_free_dyndata(void); +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, + unsigned int id, bool is_last, + XEN_GUEST_HANDLE_PARAM(void) *uaddr); +struct hypfs_entry *hypfs_gen_dyndir_id_entry( + const struct hypfs_entry_dir *template, unsigned int id, void *data); +unsigned int hypfs_dynid_entry_size(const struct hypfs_entry *template, + unsigned int id); #endif #endif /* __XEN_HYPFS_H__ */